Segher Boessenkool <seg...@kernel.crashing.org> writes: > 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?
LT traps on quiet NaNs for -ftrapping-math, UNLT and ORDERED don't. >> 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); OK, thanks, I'll do it this way. Richard
