Segher Boessenkool <seg...@kernel.crashing.org> writes:
>> find_sets_in_insn has:
>>
>> /* Don't count call-insns, (set (reg 0) (call ...)), as a set.
>> The hard function value register is used only once, to copy to
>> someplace else, so it isn't worth cse'ing. */
>> else if (GET_CODE (SET_SRC (x)) == CALL)
>> ;
>>
>> So n_sets == 1 can be true if we have a single SET in parallel
>> with a call. Unsurprising, I guess no-one had MEM sets in
>> parallel with a call, so it didn't matter until now.
>>
>> I'm retesting with a !CALL_P check added to make sure that was
>> the only problem.
>>
>> Before I resubmit, why is the simplify-rtx.c part all wrong?
>
> It was nice and simple, and it isn't anymore. 8 4 2 1 for the four of
> lt gt eq un are hardly worth documenting or making symbolic constants
> for, because a) they are familiar to all powerpc users anyway (the
> assembler has those as predefined constants!), admittedly this isn't a
> strong argument for most people;
Ah, OK. I was totally unaware of the connection.
> but also b) they were only used in two short and obvious routines.
> After your patch the routines are no longer short or obvious.
>
> A comparison result is exactly one of four things: lt, gt, eq, or un.
> So we can express testing for any subset of those with just an OR of
> the masks for the individual conditions. Whether a comparison is
> floating point, floating point no-nans, signed, or unsigned, is just
> a property of the comparison, not of the result, in this representation.
Yeah, this works for OR because we never lose the unorderdness.
There were two aspects to my patch. One was adding AND, and had:
/* We only handle AND if we can ignore unordered cases. */
bool honor_nans_p = HONOR_NANS (GET_MODE (op0));
if (code != IOR && (code != AND || honor_nans_p))
return 0;
This is needed because e.g. UNLT & ORDERED -> LT is only conditionally
valid. It doesn't sound like you're objecting to that bit, is that right?
Or was this what you had in mind with the reference to no-nans?
As mentioned in the covering note, I punted for this because handling
trapping FP comparisons correctly for AND would need its own set of
testcases. The current code punts for AND and for unsigned comparisons,
so continuing to punt for ANDs on this case seemed fair game.
> If you do not mix signed and unsigned comparisons (they make not much
> sense mixed anyway(*)), you need no changes at all: just translate
> ltu/gtu/leu/geu to/from lt/gt/le/ge on the way in and out of this
> function (there already are helper functions for that, signed_condition
> and unsigned_condition).
So this all seems to come down to whether unsigned comparisons are handled
as separate mask bits or whether they're dealt with by removing the
unsignedness and then adding it back. ISTM both are legitimate ways
of doing it. I don't think one of them is "all wrong".
FWIW, I'd posted a patch to do the IOR/AND thing in July:
https://gcc.gnu.org/ml/gcc-patches/2019-07/msg00933.html
But it turns out I'd got the signalling NaN behaviour wrong:
https://gcc.gnu.org/ml/gcc-patches/2019-08/msg01006.html
(hence punting above :-)).
I was very belatedly getting around to dealing with Joseph's comment
when you sent your patch and had it approved. Since that patch seemed
to be more favourably received in general, I was trying to work within
the existing style of your version. And without the powerpc background,
I just assumed that the mask values were "documented" by the first block
of case statements:
case LT:
return 8;
case GT:
return 4;
case EQ:
return 2;
case UNORDERED:
return 1;
Adding two more cases here didn't seem to make things any more unclear.
But maybe it is more jarring if you've memorised the powerpc mapping.
I'd actually considered converting to signed and back instead of adding
extra cases, but I thought that would be rejected as too inefficient.
(That was a concern with my patch above.) It seemed like one of the selling
points of doing it your way was that everything was handled by one switch
statement "in" and one switch statement "out", and I was trying to
preserve that.
signed_condition and unsigned_condition assert on unordered comparisons,
so if we're going to go that route, we either need to filter them out
first or add maybe_* versions of the routines that return UNKNOWN.
We also have to deal with the neutrality of EQ and NE. E.g. something
like:
rtx_code signed_code0 = maybe_signed_condition (code0);
rtx_code signed_code1 = maybe_signed_condition (code1);
if (signed_code0 != UNKNOWN && signed_code1 != UNKNOWN)
code0 = signed_code0, code1 = signed_code1;
else if (signed_code0 != UNKNOWN || signed_code1 != UNKNOWN)
return 0;
...
if (signed_code0 == UNKNOWN)
code = unsigned_condition (code);
Or (without the maybe_* versions) something like:
bool unsigned0_p
= (code0 == LTU || code0 == GTU || code0 == LEU || code0 == GEU);
bool unsigned1_p
= (code1 == LTU || code1 == GTU || code1 == LEU || code1 == GEU);
if (unsigned0_p || unsigned1_p)
{
if ((!unsigned0_p && code0 != EQ && code0 != NE)
|| (!unsigned1_p && code1 != EQ && code1 != NE))
return 0;
code0 = unsigned_condition (code0);
code1 = unsigned_condition (code1);
}
...
if (unsigned0_p)
code = unsigned_condition (code);
Which do you prefer? Or would your prefer a different way?
Another alternative would be to resurrect my patch above and try to make
the mask-to-condition conversion more efficient. Should easily be doable,
since Joseph's correction makes things simpler.
Thanks,
Richard
