Hi Richard, On Tue, Nov 19, 2019 at 07:35:10PM +0000, Richard Sandiford wrote: > Segher Boessenkool <seg...@kernel.crashing.org> writes: > > r278411 causes bootstrap to fail on at least all powerpc*. Also, I am > > author of this simplify-rtx code, and I think what you did is all wrong. > > Also, it should be two patches, the CSE part should be separate. (I can > > not tell if that is the part that regresses us, either). > > I committed them as one patch because I'd tested them together, > which I thought was the way this should be done.
Ideally you would test separate patches separately, too :-) > In practice > a clean revert would have to be done as a pair anyway: reverting > one part individually would have required XFAILs on the tests. > > And yeah, it was the cse.c part that was the problem. Thanks for finding the problem so quickly, much appreciated. > 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; 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. 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). Segher (*) lt(a,b) & ltu(a,b) means the signs of a and b are equal, and a<b (in either signedness). lt(a,b) & gtu(a,b) means a has the top bit set and b has it clear. Etc.
