| Message ID | 20150417083445.GA6178@msticlxl57.ims.intel.com |
|---|---|
| State | New |
| Headers | show |
On 04/17/2015 02:34 AM, Ilya Enkovich wrote: > On 15 Apr 14:07, Ilya Enkovich wrote: >> 2015-04-14 8:22 GMT+03:00 Jeff Law <law@redhat.com>: >>> On 03/15/2015 02:30 PM, Richard Sandiford wrote: >>>> >>>> Ilya Enkovich <enkovich.gnu@gmail.com> writes: >>>>> >>>>> This patch allows propagation of loop invariants for i386 if propagated >>>>> value is a constant to be used in address operand. Bootstrapped and >>>>> tested on x86_64-unknown-linux-gnu. OK for trunk or stage 1? >>>> >>>> >>>> Is it necessary for this to be a target hook? The concept doesn't seem >>>> particularly target-specific. We should only propagate into the address >>>> if the new cost is no greater than the old cost, but if the address >>>> meets that condition and if propagating at this point in the pipeline is >>>> a win on x86, then wouldn't it be a win for other targets too? >>> >>> I agree with Richard here. I can't see a strong reason why this should be a >>> target hook. >>> >>> Perhaps part of the issue here is the address costing metrics may not have >>> enough context to make good decisions. In which case what context do they >>> need? >> >> At this point I don't insist on a target hook. The main reasoning was >> to not affect other targets. If we extend propagation for non constant >> values different aspects may appear. E.g. possible register pressure >> changes may significantly affect ia32. I just wanted to have an >> instrument to play with a propagation on x86 not affecting other >> targets. I don't have an opportunity to test possible performance >> implications on non-x86 targets. Don't expect (significant) >> regressions there but who knows... >> >> I'll remove the hook from this patch. Will probably introduce it later >> if some target specific cases are found. >> >> Thanks, >> Ilya >> >>> >>> Jeff > > Here is a version with no hook. Bootstrapped and tested on x86_64-unknown-linux-gnu. Is it OK for trunk? > > Thanks, > Ilya > -- > gcc/ > > 2015-04-17 Ilya Enkovich <ilya.enkovich@intel.com> > > PR target/65103 > * fwprop.c (forward_propagate_into): Propagate loop > invariants if a target says so. > > gcc/testsuite/ > > 2015-04-17 Ilya Enkovich <ilya.enkovich@intel.com> > > PR target/65103 > * gcc.target/i386/pr65103-2.c: New. It seems to me there's a key piece missing here -- metrics. When is this profitable, when is it not profitable. Just blindly undoing LICM seems wrong here. The first thought is to look at register pressure through the loop. I thought we had some infrastructure for this kind of query available. It'd probably be wise to re-use it. In fact, one might reasonably ask if LICM should have hoisted the expression to start with. I'd also think the cost of the constant may come into play here. A really cheap constant probably should not have been hoisted by LICM to start with -- but the code may have been written in such a way that some low cost constants are pulled out as loop invariants at the source level. So this isn't strictly an issue of un-doing bad LICM So I think to go forward we need to be working on solving the "when is this a profitable transformation to make". jeff
2015-04-21 8:52 GMT+03:00 Jeff Law <law@redhat.com>: > On 04/17/2015 02:34 AM, Ilya Enkovich wrote: >> >> On 15 Apr 14:07, Ilya Enkovich wrote: >>> >>> 2015-04-14 8:22 GMT+03:00 Jeff Law <law@redhat.com>: >>>> >>>> On 03/15/2015 02:30 PM, Richard Sandiford wrote: >>>>> >>>>> >>>>> Ilya Enkovich <enkovich.gnu@gmail.com> writes: >>>>>> >>>>>> >>>>>> This patch allows propagation of loop invariants for i386 if >>>>>> propagated >>>>>> value is a constant to be used in address operand. Bootstrapped and >>>>>> tested on x86_64-unknown-linux-gnu. OK for trunk or stage 1? >>>>> >>>>> >>>>> >>>>> Is it necessary for this to be a target hook? The concept doesn't seem >>>>> particularly target-specific. We should only propagate into the >>>>> address >>>>> if the new cost is no greater than the old cost, but if the address >>>>> meets that condition and if propagating at this point in the pipeline >>>>> is >>>>> a win on x86, then wouldn't it be a win for other targets too? >>>> >>>> >>>> I agree with Richard here. I can't see a strong reason why this should >>>> be a >>>> target hook. >>>> >>>> Perhaps part of the issue here is the address costing metrics may not >>>> have >>>> enough context to make good decisions. In which case what context do >>>> they >>>> need? >>> >>> >>> At this point I don't insist on a target hook. The main reasoning was >>> to not affect other targets. If we extend propagation for non constant >>> values different aspects may appear. E.g. possible register pressure >>> changes may significantly affect ia32. I just wanted to have an >>> instrument to play with a propagation on x86 not affecting other >>> targets. I don't have an opportunity to test possible performance >>> implications on non-x86 targets. Don't expect (significant) >>> regressions there but who knows... >>> >>> I'll remove the hook from this patch. Will probably introduce it later >>> if some target specific cases are found. >>> >>> Thanks, >>> Ilya >>> >>>> >>>> Jeff >> >> >> Here is a version with no hook. Bootstrapped and tested on >> x86_64-unknown-linux-gnu. Is it OK for trunk? >> >> Thanks, >> Ilya >> -- >> gcc/ >> >> 2015-04-17 Ilya Enkovich <ilya.enkovich@intel.com> >> >> PR target/65103 >> * fwprop.c (forward_propagate_into): Propagate loop >> invariants if a target says so. >> >> gcc/testsuite/ >> >> 2015-04-17 Ilya Enkovich <ilya.enkovich@intel.com> >> >> PR target/65103 >> * gcc.target/i386/pr65103-2.c: New. > > It seems to me there's a key piece missing here -- metrics. > > When is this profitable, when is it not profitable. Just blindly undoing > LICM seems wrong here. > > The first thought is to look at register pressure through the loop. I > thought we had some infrastructure for this kind of query available. It'd > probably be wise to re-use it. In fact, one might reasonably ask if LICM > should have hoisted the expression to start with. > > > I'd also think the cost of the constant may come into play here. A really > cheap constant probably should not have been hoisted by LICM to start with > -- but the code may have been written in such a way that some low cost > constants are pulled out as loop invariants at the source level. So this > isn't strictly an issue of un-doing bad LICM > > So I think to go forward we need to be working on solving the "when is this > a profitable transformation to make". This patch doesn't force propagation. The patch just allows propagation and regular fwprop cost estimation is used to compute if this is profitable. For i386 I don't see cases when we shouldn't propagate. We remove instruction, reduce register pressure and having constant in memory operand is free which is reflected in address_cost hook. Ilya > > jeff
Ping 2015-05-05 14:05 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>: > 2015-04-21 8:52 GMT+03:00 Jeff Law <law@redhat.com>: >> On 04/17/2015 02:34 AM, Ilya Enkovich wrote: >>> >>> On 15 Apr 14:07, Ilya Enkovich wrote: >>>> >>>> 2015-04-14 8:22 GMT+03:00 Jeff Law <law@redhat.com>: >>>>> >>>>> On 03/15/2015 02:30 PM, Richard Sandiford wrote: >>>>>> >>>>>> >>>>>> Ilya Enkovich <enkovich.gnu@gmail.com> writes: >>>>>>> >>>>>>> >>>>>>> This patch allows propagation of loop invariants for i386 if >>>>>>> propagated >>>>>>> value is a constant to be used in address operand. Bootstrapped and >>>>>>> tested on x86_64-unknown-linux-gnu. OK for trunk or stage 1? >>>>>> >>>>>> >>>>>> >>>>>> Is it necessary for this to be a target hook? The concept doesn't seem >>>>>> particularly target-specific. We should only propagate into the >>>>>> address >>>>>> if the new cost is no greater than the old cost, but if the address >>>>>> meets that condition and if propagating at this point in the pipeline >>>>>> is >>>>>> a win on x86, then wouldn't it be a win for other targets too? >>>>> >>>>> >>>>> I agree with Richard here. I can't see a strong reason why this should >>>>> be a >>>>> target hook. >>>>> >>>>> Perhaps part of the issue here is the address costing metrics may not >>>>> have >>>>> enough context to make good decisions. In which case what context do >>>>> they >>>>> need? >>>> >>>> >>>> At this point I don't insist on a target hook. The main reasoning was >>>> to not affect other targets. If we extend propagation for non constant >>>> values different aspects may appear. E.g. possible register pressure >>>> changes may significantly affect ia32. I just wanted to have an >>>> instrument to play with a propagation on x86 not affecting other >>>> targets. I don't have an opportunity to test possible performance >>>> implications on non-x86 targets. Don't expect (significant) >>>> regressions there but who knows... >>>> >>>> I'll remove the hook from this patch. Will probably introduce it later >>>> if some target specific cases are found. >>>> >>>> Thanks, >>>> Ilya >>>> >>>>> >>>>> Jeff >>> >>> >>> Here is a version with no hook. Bootstrapped and tested on >>> x86_64-unknown-linux-gnu. Is it OK for trunk? >>> >>> Thanks, >>> Ilya >>> -- >>> gcc/ >>> >>> 2015-04-17 Ilya Enkovich <ilya.enkovich@intel.com> >>> >>> PR target/65103 >>> * fwprop.c (forward_propagate_into): Propagate loop >>> invariants if a target says so. >>> >>> gcc/testsuite/ >>> >>> 2015-04-17 Ilya Enkovich <ilya.enkovich@intel.com> >>> >>> PR target/65103 >>> * gcc.target/i386/pr65103-2.c: New. >> >> It seems to me there's a key piece missing here -- metrics. >> >> When is this profitable, when is it not profitable. Just blindly undoing >> LICM seems wrong here. >> >> The first thought is to look at register pressure through the loop. I >> thought we had some infrastructure for this kind of query available. It'd >> probably be wise to re-use it. In fact, one might reasonably ask if LICM >> should have hoisted the expression to start with. >> >> >> I'd also think the cost of the constant may come into play here. A really >> cheap constant probably should not have been hoisted by LICM to start with >> -- but the code may have been written in such a way that some low cost >> constants are pulled out as loop invariants at the source level. So this >> isn't strictly an issue of un-doing bad LICM >> >> So I think to go forward we need to be working on solving the "when is this >> a profitable transformation to make". > > This patch doesn't force propagation. The patch just allows > propagation and regular fwprop cost estimation is used to compute if > this is profitable. For i386 I don't see cases when we shouldn't > propagate. We remove instruction, reduce register pressure and having > constant in memory operand is free which is reflected in address_cost > hook. > > Ilya > >> >> jeff
2015-05-29 1:15 GMT+03:00 Jeff Law <law@redhat.com>: > > Right, but you're blindly propagating. The right thing to do is look at > some kind of metric to estimate when it's profitable to propagate the > constant back in vs leave it hoisted out. No, the patch is not to blindly propagate but to let loop invariant to be propagated into address. Existing propagation gain estimation (should_replace_address) still applies. Thanks, Ilya > > If you look at what Kugan is doing in cprop.c, that's exactly the approach > he's taking -- looking at rtx costing to determine when to propagate the > constant back into the loop. It could probably be made better with some > knowledge of register pressure and looking at whether or not all uses (vs > just some uses) of the constant will be propagated. > > Jeff
Ping 2015-06-01 15:26 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>: > 2015-05-29 1:15 GMT+03:00 Jeff Law <law@redhat.com>: >> >> Right, but you're blindly propagating. The right thing to do is look at >> some kind of metric to estimate when it's profitable to propagate the >> constant back in vs leave it hoisted out. > > No, the patch is not to blindly propagate but to let loop invariant to > be propagated into address. Existing propagation gain estimation > (should_replace_address) still applies. > > Thanks, > Ilya > >> >> If you look at what Kugan is doing in cprop.c, that's exactly the approach >> he's taking -- looking at rtx costing to determine when to propagate the >> constant back into the loop. It could probably be made better with some >> knowledge of register pressure and looking at whether or not all uses (vs >> just some uses) of the constant will be propagated. >> >> Jeff
On 06/01/2015 06:26 AM, Ilya Enkovich wrote: > 2015-05-29 1:15 GMT+03:00 Jeff Law <law@redhat.com>: >> >> Right, but you're blindly propagating. The right thing to do is look at >> some kind of metric to estimate when it's profitable to propagate the >> constant back in vs leave it hoisted out. > > No, the patch is not to blindly propagate but to let loop invariant to > be propagated into address. Existing propagation gain estimation > (should_replace_address) still applies. Agreed. I missed the costing metric in should_replace_address. Sorry for not looking more deeply at that and delaying progress on this issue. So looking more closely at the patch itself... AFAICT, your change to that condition potentially allows fwprop to propagate any constants to any use, regardless of context (mem vs non-mem). You're largely getting away with that because you're checking for the rtx code CONST -- which filters out everything except symbolic constants. ie, you're filtering out CONST_INT, CONST_DOUBLE, CONST_*. So the first thing you need to do is clarify that comment a bit. Perhaps: /* Do not propagate loop invariant definitions into a different loop, except for symbolic constants which may propagate, subject to cost testing. */ The second issue is I think the testcase has been compromised. If I compile the test on the trunk, I don't see any references to GOTOFF. So I think you need to create a new testcase. One way would be to instrument a build, detecting anytime you were able to propagate a symbolic constant into a different loop nest -- if you can discover one, then use creduce or multidelta to produce a simplified testcase. Because of the CONST vs CONST_INT stuff, you're not running the risk of regressing 65768, so no need to build an arm cross a test that :-) With the comment update and a new testcase this is OK for the trunk. Again, sorry for delaying progress on this. Jeff
diff --git a/gcc/fwprop.c b/gcc/fwprop.c index fc64ec9..82ebd01 100644 --- a/gcc/fwprop.c +++ b/gcc/fwprop.c @@ -1365,8 +1365,18 @@ forward_propagate_into (df_ref use) if (DF_REF_IS_ARTIFICIAL (def)) return false; - /* Do not propagate loop invariant definitions inside the loop. */ - if (DF_REF_BB (def)->loop_father != DF_REF_BB (use)->loop_father) + def_insn = DF_REF_INSN (def); + if (multiple_sets (def_insn)) + return false; + def_set = single_set (def_insn); + if (!def_set) + return false; + + /* Do not propagate loop invariant definitions inside the loop. + Allow address constant propagation. */ + if (DF_REF_BB (def)->loop_father != DF_REF_BB (use)->loop_father + && (DF_REF_TYPE (use) == DF_REF_REG_USE + || GET_CODE (SET_SRC (def_set)) != CONST)) return false; /* Check if the use is still present in the insn! */ @@ -1379,13 +1389,6 @@ forward_propagate_into (df_ref use) if (!reg_mentioned_p (DF_REF_REG (use), parent)) return false; - def_insn = DF_REF_INSN (def); - if (multiple_sets (def_insn)) - return false; - def_set = single_set (def_insn); - if (!def_set) - return false; - /* Only try one kind of propagation. If two are possible, we'll do it on the following iterations. */ if (forward_propagate_and_simplify (use, def_insn, def_set) diff --git a/gcc/testsuite/gcc.target/i386/pr65103-2.c b/gcc/testsuite/gcc.target/i386/pr65103-2.c new file mode 100644 index 0000000..b7a32f7 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr65103-2.c @@ -0,0 +1,24 @@ +/* { dg-do compile { target ia32 } } */ +/* { dg-require-effective-target pie } */ +/* { dg-options "-O2 -fPIE" } */ +/* { dg-final { scan-assembler-not "GOTOFF," } } */ + +typedef struct S +{ + int a; + int b; +} S; +struct S gs; + +extern int compute ( struct S * ); + +int test( void ) +{ + int t = -1; + while (t) + { + gs.a++; + t = compute (&gs); + } + return 0; +}