Richard Sandiford <richard.sandif...@arm.com> writes: > 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);
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