Reverting r278411

[Segher Boessenkool]

Hi!

On Wed, Nov 20, 2019 at 09:42:46AM +0000, Richard Sandiford wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
> >> 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.

Yeah, I should have documented it.  Time pressure was high the last N
weeks, with everyone else putting stuff in before end of stage 1 things
broke left and right for me.  Normally I would just not update trunk the
last two months or so of stage 1 (for development -- for testing you
always need current trunk of course), but I needed some stuff from it.
Oh well, I finally dug myself out.

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

It works for everything, including things with a NOT.

If the mode does not allow unordered, you mask that away all the way at
the end, and nothing else needs to be done.  This doesn't have to be done
for IOR because you can never end up with unordered in the mask if you
didn't start out with some code that allows unordered.

You also can encode NE as not allowing unordered, if you have a mode
where that does not exist, but that is purely cosmetic.

8421  "full"     "no-nan"
0000  false      false
1000  lt         lt
0100  gt         gt
1100  ltgt       ne
0010  eq         eq
1010  le         le
0110  ge         ge
1110  ordered    true
0001  unordered
1001  unlt
0101  ungt
1101  ne
0011  uneq
1011  unle
0111  unge
1111  true

So for !HONOR_NANS modes we need to output LTGT as NE, and ORDERED as
true, and that is all.

> 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?

UNLT & ORDERED is always LT.  When would it not be true?

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

This isn't trapping arithmetic.  Unordered is a perfectly normal result.
As IEEE 754 says:
  Four mutually exclusive relations are possible: less than, equal,
  greater than, and unordered.
This same is true when !HONOR_NANS, for signed integer comparisons, and
for unsigned integer comparisons: just UNORDERED never happens.

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

It violates the whole design of the thing left and right.  I never
documented that well (or at all), of course :-/

> 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;

Yeah, but it may not be obvious that exactly one of those is true for
any comparison, and you can combine them to a mask and do any logical
operations on it.

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

It's also that 2**4 is small, but 2**6 is not.  The 2**4 cases all have
separate RTL codes for it, too.

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

Yeah.  rs6000 has

(define_predicate "unsigned_comparison_operator"
  (match_code "ltu,gtu,leu,geu"))
(define_predicate "signed_comparison_operator"
  (match_code "lt,gt,le,ge"))

Maybe we should have those be for every target?

  bool is_signed = (signed_comparison_operator (code0)
                    || signed_comparison_operator (code1));
  bool is_unsigned = (unsigned_comparison_operator (code0)
                      || unsigned_comparison_operator (code1));

  /* Don't allow mixing signed and unsigned comparisons.  */
  if (is_signed && is_unsigned)
    return 0;

(this takes care of your EQ/NE concern automatically, btw)

  if (unsigned_comparison_operator (code0))
    code0 = signed_condition (code0);
  if (unsigned_comparison_operator (code1))
    code1 = signed_condition (code1);

and at the end

  if (is_unsigned && signed_comparison_operator (code))
    code = unsigned_condition (code);


Segher