Hi! On Wed, Nov 20, 2019 at 09:42:46AM +0000, Richard Sandiford wrote: > Segher Boessenkool <seg...@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