Message ID | fb18e1bc-7cdb-9227-55e5-01298e11b9c3@gjlay.de |
---|---|
State | New |
Headers | show |
Hi, On Tue, Jul 11, 2017 at 10:47:27AM +0200, Georg-Johann Lay wrote: > This small addition improves costs of PARALLELs in > rtlanal.c:seq_cost(). Up to now, these costs are > assumed to be 1 which gives gross inexact costs for, > e.g. divmod which is represented as PARALLEL. insn_rtx_cost returns 0 ("unknown") for such a PARALLEL, so your current patch does not change this at all? > --- rtlanal.c (revision 248745) > +++ rtlanal.c (working copy) > @@ -5300,6 +5300,9 @@ seq_cost (const rtx_insn *seq, bool spee > set = single_set (seq); > if (set) > cost += set_rtx_cost (set, speed); So, why does this not use insn_rtx_cost as well? > + else if (INSN_P (seq) > + && PARALLEL == GET_CODE (PATTERN (seq))) Yoda conditions have we should not. > + cost += 1 + insn_rtx_cost (PATTERN (seq), speed); > else > cost++; > } This whole thing could be something like if (INSN_P (seq)) { int t = insn_rtx_cost (PATTERN (seq), speed); cost += t ? t : COST_N_INSNS (1); } else cost++; But set_rtx_cost does *not* return the same answer as insn_rtx_cost. (Why do you need a check for INSN_P here? Why does it increment the cost for non-insns? So many questions). Segher
On 12.07.2017 14:11, Segher Boessenkool wrote: > Hi, > > On Tue, Jul 11, 2017 at 10:47:27AM +0200, Georg-Johann Lay wrote: >> This small addition improves costs of PARALLELs in >> rtlanal.c:seq_cost(). Up to now, these costs are >> assumed to be 1 which gives gross inexact costs for, >> e.g. divmod which is represented as PARALLEL. > > insn_rtx_cost returns 0 ("unknown") for such a PARALLEL, so your > current patch does not change this at all? Huh? It returns the costs of 1st SET in a PARALLEL (provided it has one), no? Or even costs for come compares. >> --- rtlanal.c (revision 248745) >> +++ rtlanal.c (working copy) >> @@ -5300,6 +5300,9 @@ seq_cost (const rtx_insn *seq, bool spee >> set = single_set (seq); >> if (set) >> cost += set_rtx_cost (set, speed); > > So, why does this not use insn_rtx_cost as well? You'll have to ask the author of that line... And I didn't intend to change existing behaviour except for a case where I know that "1" is far off the real costs. > >> + else if (INSN_P (seq) >> + && PARALLEL == GET_CODE (PATTERN (seq))) > > Yoda conditions have we should not. hmm, I didn't find something like PARALLEL_P (rtx). Is comparing rtx_codes deprecated now? >> + cost += 1 + insn_rtx_cost (PATTERN (seq), speed); >> else >> cost++; >> } > > This whole thing could be something like > > if (INSN_P (seq)) > { > int t = insn_rtx_cost (PATTERN (seq), speed); This will behave differently. The original set_src_cost calls rtx_costs with SET and outer_code = INSN, insn_rtx_cost does not. My intentions was to be conservative and not silently introduce performance degradations in whatever back-end by changing the seen RTXes or codes. Everything that rtx_costs was called for will be the same. Nothing changes, just some new calls are added. But neither are existing calls removed nor are ones changes to up different arguments. > cost += t ? t : COST_N_INSNS (1); > } > else > cost++; > > But set_rtx_cost does *not* return the same answer as insn_rtx_cost. > > (Why do you need a check for INSN_P here? Why does it increment the > cost for non-insns? So many questions). Again, you'll have to ask the original author for reasoning. Johann > > > Segher >
On Wed, Jul 12, 2017 at 03:30:00PM +0200, Georg-Johann Lay wrote: > On 12.07.2017 14:11, Segher Boessenkool wrote: > >On Tue, Jul 11, 2017 at 10:47:27AM +0200, Georg-Johann Lay wrote: > >>This small addition improves costs of PARALLELs in > >>rtlanal.c:seq_cost(). Up to now, these costs are > >>assumed to be 1 which gives gross inexact costs for, > >>e.g. divmod which is represented as PARALLEL. > > > >insn_rtx_cost returns 0 ("unknown") for such a PARALLEL, so your > >current patch does not change this at all? > > Huh? It returns the costs of 1st SET in a PARALLEL (provided it > has one), no? Or even costs for come compares. No, it returns 0 if there is more than one normal SET (or more than one compare). > >>+ else if (INSN_P (seq) > >>+ && PARALLEL == GET_CODE (PATTERN (seq))) > > > >Yoda conditions have we should not. > > hmm, I didn't find something like PARALLEL_P (rtx). > Is comparing rtx_codes deprecated now? I meant it should be written else if (INSN_P (seq) && GET_CODE (PATTERN (seq)) == PARALLEL) i.e. constant on the right. > >This whole thing could be something like > > > > if (INSN_P (seq)) > > { > > int t = insn_rtx_cost (PATTERN (seq), speed); > > This will behave differently. Yes, I know, I even said so. > >(Why do you need a check for INSN_P here? Why does it increment the > > >cost for non-insns? So many questions). > > Again, you'll have to ask the original author for reasoning. Since you want to change the code, to make it better, I was hoping you would dig in a bit. To make it better, not just different :-/ Segher
On 12.07.2017 21:36, Segher Boessenkool wrote: > On Wed, Jul 12, 2017 at 03:30:00PM +0200, Georg-Johann Lay wrote: >> On 12.07.2017 14:11, Segher Boessenkool wrote: >>> On Tue, Jul 11, 2017 at 10:47:27AM +0200, Georg-Johann Lay wrote: >>>> This small addition improves costs of PARALLELs in >>>> rtlanal.c:seq_cost(). Up to now, these costs are >>>> assumed to be 1 which gives gross inexact costs for, >>>> e.g. divmod which is represented as PARALLEL. >>> >>> insn_rtx_cost returns 0 ("unknown") for such a PARALLEL, so your >>> current patch does not change this at all? Ah I see now. So this is unfixable... Any change to seq_cost that would address the issue I had in mind (completely wrong costs for divmod that are represented as PARALLEL with 2 SETs) will come up with different handling than the "logic" of insn_rtx_costs. So the only way to avoid that "logic" is to pass the whole story to the back-end. And in order not to break any existing assumptions this can only be achieved by a new hook that graceful degrades to the current behaviour and reasoning when that new hook says "dunno". I already started an RFC here: https://gcc.gnu.org/ml/gcc/2017-07/msg00080.html Johann
Index: rtlanal.c =================================================================== --- rtlanal.c (revision 248745) +++ rtlanal.c (working copy) @@ -5300,6 +5300,9 @@ seq_cost (const rtx_insn *seq, bool spee set = single_set (seq); if (set) cost += set_rtx_cost (set, speed); + else if (INSN_P (seq) + && PARALLEL == GET_CODE (PATTERN (seq))) + cost += 1 + insn_rtx_cost (PATTERN (seq), speed); else cost++; }