On Wed, Sep 16, 2020 at 07:02:06PM -0500, Segher Boessenkool wrote: > Hi! > > On Tue, Sep 15, 2020 at 10:49:44AM +0930, Alan Modra wrote: > > * config/rs6000/rs6000.c (rs6000_rtx_costs): Cost IOR. > > > case IOR: > > - /* FIXME */ > > *total = COSTS_N_INSNS (1); > > - return true; > > Hey this was okay for over five years :-) > > > + left = XEXP (x, 0); > > + if (GET_CODE (left) == AND > > + && CONST_INT_P (XEXP (left, 1))) > > Add a comment that this is the integer insert insns? > > > + // rotlsi3_insert_5 > > But use /* comments */. > > > + /* Test both regs even though the one in the mask is > > + constrained to be equal to the output. Increasing > > + cost may well result in rejecting an invalid insn > > + earlier. */ > > Is that ever actually useful?
Possibly not in this particular case, but I did see cases where invalid insns were rejected early by costing non-reg sub-expressions. Beside that, the default position on rtx_costs paths that return true should be to cost any sub-expressions unless you know for sure they are zero cost. And yes, we fail to do that for some cases, eg. mul_highpart. > So this new block is pretty huge. Can it easily be factored to a > separate function? Just the insert insns part, not all IOR. Done in my local tree. > Okay for trunk with the comments changed to the correct syntax, and > factoring masked insert out to a separate function pre-approved if you > want to do that. Thanks! I'll hold off committing until the whole rtx_costs patch series is reviewed (not counting the rotate_and_mask_constant patch). -- Alan Modra Australia Development Lab, IBM
