| Message ID | 9b57c474-b33b-4a89-82f2-f9a33b1810df@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | rtlanal: Correct cost regularization in pattern_cost | expand |
Hi, 在 2024/5/10 20:50, Richard Biener 写道: > IMO give we're dispatching to the rtx_cost hook eventually it needs > documenting there or alternatively catching zero and adjusting its > result there. Of course cost == 0 ? 1 : cost is wrong as it makes > zero vs. one the same cost - using cost + 1 when from rtx_cost > might be more correct, at least preserving relative costs. I tested the draft patch which sets "cost > 0 ? cost + 1 : 1;". Some regression cases are found on x86. The main problems are: The cost compare with COSTS_N_INSNS (1) doesn't works any more with the patch. As all costs are added with 1, the following compare returns true when the cost is 5 but false originally. if (cost > COSTS_N_INSNS (1)) Another problem is the cost is from set_src_cost, it doesn't take dest into consideration. For example, the cost of a store "[`x']=r109:SI" is set to 1 as it only measure the cost of set_src. It seems unreasonable. IMHO, the cost less than COSTS_N_INSNS (1) is meaningful in rtx_cost calculation but unreasonable for an insn. Should the minimum cost of an insn be set to COSTS_N_INSNS (1)? Thanks Gui Haochen
Richard Biener <richard.guenther@gmail.com> writes: > On Fri, May 10, 2024 at 4:25 AM HAO CHEN GUI <guihaoc@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
diff --git a/gcc/rtlanal.cc b/gcc/rtlanal.cc index 4158a531bdd..f7b3d7d72ce 100644 --- a/gcc/rtlanal.cc +++ b/gcc/rtlanal.cc @@ -5762,7 +5762,7 @@ pattern_cost (rtx pat, bool speed) return 0; cost = set_src_cost (SET_SRC (set), GET_MODE (SET_DEST (set)), speed); - return cost > 0 ? cost : COSTS_N_INSNS (1); + return cost > COSTS_N_INSNS (1) ? cost : COSTS_N_INSNS (1); } /* Calculate the cost of a single instruction. A return value of zero
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? Thanks Gui Haochen ChangeLog rtlanal: Correct cost regularization in pattern_cost For the pattern_cost (insn_cost), the smallest known cost is COSTS_N_INSNS (1) and zero means the cost is unknown. The method calls set_src_cost which might returns 0 or a value less than COSTS_N_INSNS (1). For these cases, pattern_cost should always return COSTS_N_INSNS (1). Current regularization is wrong and a value less than COSTS_N_INSNS (1) but larger than 0 will be returned. This patch corrects it. gcc/ * rtlanal.cc (pattern_cost): Return COSTS_N_INSNS (1) when the cost is less than COSTS_N_INSNS (1). patch.diff