Re: Reverting r278411

[Richard Sandiford <richard dot sandiford at arm dot com>]

Richard Sandiford <richard.sandiford@arm.com> writes:
> Segher Boessenkool <segher@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);

Of course I meant signed_condition here :-)

>     }
>
>   ...
>
>   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