Richard Biener <richard.guent...@gmail.com> writes: > On Fri, May 10, 2024 at 4:25 AM HAO CHEN GUI <guih...@linux.ibm.com> wrote: >> >> Hi, >> The cost return from set_src_cost might be zero. Zero for >> pattern_cost means unknown cost. So the regularization converts the zero >> to COSTS_N_INSNS (1). >> >> // pattern_cost >> cost = set_src_cost (SET_SRC (set), GET_MODE (SET_DEST (set)), speed); >> return cost > 0 ? cost : COSTS_N_INSNS (1); >> >> But if set_src_cost returns a value less than COSTS_N_INSNS (1), it's >> untouched and just returned by pattern_cost. Thus "zero" from set_src_cost >> is higher than "one" from set_src_cost. >> >> For instance, i386 returns cost "one" for zero_extend op. >> //ix86_rtx_costs >> case ZERO_EXTEND: >> /* The zero extensions is often completely free on x86_64, so make >> it as cheap as possible. */ >> if (TARGET_64BIT && mode == DImode >> && GET_MODE (XEXP (x, 0)) == SImode) >> *total = 1; >> >> This patch fixes the problem by converting all costs which are less than >> COSTS_N_INSNS (1) to COSTS_N_INSNS (1). >> >> Bootstrapped and tested on x86 and powerpc64-linux BE and LE with no >> regressions. Is it OK for the trunk? > > But if targets return sth < COSTS_N_INSNS (1) but > 0 this is now no > longer meaningful. So shouldn't it instead be > > return cost > 0 ? cost : 1; > > ? Alternatively returning fractions of COSTS_N_INSNS (1) from set_src_cost > is invalid and thus the target is at fault (I do think that making zero the > unknown value is quite bad since that makes it impossible to have zero > as cost represented).
I agree zero is an unfortunate choice. No-op moves should really have zero cost, without having to be special-cased by callers. And it came as a surprise to me that we had this rule. But like Segher says, it seems to have been around for a long time (since 2004 by the looks of it, r0-59417). Which just goes to show, every day is a learning day. :) IMO it would be nice to change it. But then it would be even nicer to get rid of pattern_cost and move everything to insn_cost. And that's going to be a lot of work to do together. Maybe a compromise would be to open-code pattern_cost into insn_cost and change the return value for insn_cost only? That would still mean auditing all current uses of insn_cost and all current target definitions of the insn_cost hook, but at least it would be isolated from the work of removing pattern_cost. Thanks, Richard
