[PR65768] Check rtx_cost when propagating constant

[PR65768] Check rtx_cost when propagating constant

[Kugan]

[-- Attachment #1: Type: text/plain, Size: 625 bytes --]

As mentioned in PR65768, ARM gcc generates suboptimal code for constant
Uses in loop. Part of the reason is cprop is undoing what loop invariant
code motion did.

Zhenqiang posted a patch at to fix this based on rtx costs:
https://gcc.gnu.org/ml/gcc-patches/2014-06/msg01321.html

I cleaned it up and bootstrapped, regression tested on x86_64-linux-gnu;
no new regressions. Is this OK for trunk?

Thanks,
Kugan

gcc/ChangeLog:

2015-04-15  Kugan Vivekanandarajah  <kuganv@linaro.org>
	    Zhenqiang Chen  <zhenqiang.chen@linaro.org>

	PR target/65768
	* cprop.c (try_replace_reg): Check cost of constants before propagating.

[-- Attachment #2: cprop.txt --]
[-- Type: text/plain, Size: 1289 bytes --]

diff --git a/gcc/cprop.c b/gcc/cprop.c
index c9fb2fc..42a2a72 100644
--- a/gcc/cprop.c
+++ b/gcc/cprop.c
@@ -758,12 +758,38 @@ try_replace_reg (rtx from, rtx to, rtx_insn *insn)
   int success = 0;
   rtx set = single_set (insn);
 
+  bool already_const_p = false;
+  bool speed = optimize_bb_for_speed_p (BLOCK_FOR_INSN (insn));
+  int old_cost = set ? set_rtx_cost (set, speed) : 0;
+
+  if ((note != 0
+      && REG_NOTE_KIND (note) == REG_EQUAL
+      && (GET_CODE (XEXP (note, 0)) == CONST
+	  || CONSTANT_P (XEXP (note, 0))))
+      || (set && CONSTANT_P (SET_SRC (set))))
+    already_const_p = true;
+
   /* Usually we substitute easy stuff, so we won't copy everything.
      We however need to take care to not duplicate non-trivial CONST
      expressions.  */
   to = copy_rtx (to);
 
   validate_replace_src_group (from, to, insn);
+
+
+  /* For CONSTANT_P (to), loop2_invariant pass might hoist it out the loop.
+     And it can be shared by different references.  So skip propagation if
+     it makes INSN's rtx cost higher.  */
+
+  if (!already_const_p
+      && CONSTANT_P (to)
+      && (set_rtx_cost (set, speed) > old_cost))
+    {
+      cancel_changes (0);
+      return false;
+    }
+
+
   if (num_changes_pending () && apply_change_group ())
     success = 1;
 

Re: [PR65768] Check rtx_cost when propagating constant

[Steven Bosscher]

On Wed, Apr 15, 2015 at 9:53 AM, Kugan wrote:
> 2015-04-15  Kugan Vivekanandarajah  < >
>             Zhenqiang Chen  <>
>
>         PR target/65768
>         * cprop.c (try_replace_reg): Check cost of constants before propagating.


> +
> +  /* For CONSTANT_P (to), loop2_invariant pass might hoist it out the loop.
> +     And it can be shared by different references.  So skip propagation if
> +     it makes INSN's rtx cost higher.  */
> +

So only undo if the insn is inside a loop (i.e.
BLOCK_FOR_INSN(insn)->loop_father != NULL) and this is a
post-pass_loop2 cprop run?

Ciao!
Steven

Re: [PR65768] Check rtx_cost when propagating constant

[Richard Biener]

On Wed, Apr 15, 2015 at 11:05 AM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> On Wed, Apr 15, 2015 at 9:53 AM, Kugan wrote:
>> 2015-04-15  Kugan Vivekanandarajah  < >
>>             Zhenqiang Chen  <>
>>
>>         PR target/65768
>>         * cprop.c (try_replace_reg): Check cost of constants before propagating.
>
>
>> +
>> +  /* For CONSTANT_P (to), loop2_invariant pass might hoist it out the loop.
>> +     And it can be shared by different references.  So skip propagation if
>> +     it makes INSN's rtx cost higher.  */
>> +
>
> So only undo if the insn is inside a loop (i.e.
> BLOCK_FOR_INSN(insn)->loop_father != NULL) and this is a
> post-pass_loop2 cprop run?

post loop2 loops are destroyed.  When loops are available loop_father
is always non-NULL, the proper check is for loop_outer (->loop_father) == NULL.
or loop_depth (->loop_father) != 0.

Richard.

>
> Ciao!
> Steven

Re: [PR65768] Check rtx_cost when propagating constant

[Kugan]

On 15/04/15 21:18, Richard Biener wrote:
> On Wed, Apr 15, 2015 at 11:05 AM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
>> On Wed, Apr 15, 2015 at 9:53 AM, Kugan wrote:
>>> 2015-04-15  Kugan Vivekanandarajah  < >
>>>             Zhenqiang Chen  <>
>>>
>>>         PR target/65768
>>>         * cprop.c (try_replace_reg): Check cost of constants before propagating.
>>
>>
>>> +
>>> +  /* For CONSTANT_P (to), loop2_invariant pass might hoist it out the loop.
>>> +     And it can be shared by different references.  So skip propagation if
>>> +     it makes INSN's rtx cost higher.  */
>>> +
>>
>> So only undo if the insn is inside a loop (i.e.
>> BLOCK_FOR_INSN(insn)->loop_father != NULL) and this is a
>> post-pass_loop2 cprop run?
> 
> post loop2 loops are destroyed.  When loops are available loop_father
> is always non-NULL, the proper check is for loop_outer (->loop_father) == NULL.
> or loop_depth (->loop_father) != 0.

Thanks Steven and Richard for the comments. If the loop information is
present, we could have used this. But even otherwise, we are just
limiting the cprop of an expensive constant (based on the rtx_cost). I
understand that Richard was a bit concerned about extending the live
range but this is a trade off. As per his previous mail, Zhenqiang did
some benchmarking. I am happy to do further benchmarking if you want to
see that.

Probably the rematerialization that is being introduced in IRA/LRA can
redo this if it sees there is high register pressure. Any thoughts?

Thanks,
Kugan

Re: [PR65768] Check rtx_cost when propagating constant

[Kugan]

ping?

Thanks,
Kugan

On 15/04/15 17:53, Kugan wrote:
> As mentioned in PR65768, ARM gcc generates suboptimal code for constant
> Uses in loop. Part of the reason is cprop is undoing what loop invariant
> code motion did.
> 
> Zhenqiang posted a patch at to fix this based on rtx costs:
> https://gcc.gnu.org/ml/gcc-patches/2014-06/msg01321.html
> 
> I cleaned it up and bootstrapped, regression tested on x86_64-linux-gnu;
> no new regressions. Is this OK for trunk?
> 
> Thanks,
> Kugan
> 
> gcc/ChangeLog:
> 
> 2015-04-15  Kugan Vivekanandarajah  <kuganv@linaro.org>
> 	    Zhenqiang Chen  <zhenqiang.chen@linaro.org>
> 
> 	PR target/65768
> 	* cprop.c (try_replace_reg): Check cost of constants before propagating.
> 

Re: [PR65768] Check rtx_cost when propagating constant

[Jeff Law]

I've CC'd Ilya as he's been looking at related issues in the x86 
backend, but from the other direction and I think he ought to be aware 
of the interactions of this potential change and his work.  In 
particular depending on the costing in the x86 backend we may see fewer 
propagations of GOTOFF constants to their use sites.



On 05/13/2015 11:46 PM, Kugan wrote:
> ping?
>
> Thanks,
> Kugan
>
> On 15/04/15 17:53, Kugan wrote:
>> As mentioned in PR65768, ARM gcc generates suboptimal code for constant
>> Uses in loop. Part of the reason is cprop is undoing what loop invariant
>> code motion did.
>>
>> Zhenqiang posted a patch at to fix this based on rtx costs:
>> https://gcc.gnu.org/ml/gcc-patches/2014-06/msg01321.html
>>
>> I cleaned it up and bootstrapped, regression tested on x86_64-linux-gnu;
>> no new regressions. Is this OK for trunk?
>>
>> Thanks,
>> Kugan
>>
>> gcc/ChangeLog:
>>
>> 2015-04-15  Kugan Vivekanandarajah  <kuganv@linaro.org>
>> 	    Zhenqiang Chen  <zhenqiang.chen@linaro.org>
>>
>> 	PR target/65768
>> 	* cprop.c (try_replace_reg): Check cost of constants before propagating.
So, I've reviewed the discussion from last year.  To summarize my 
understanding (please correct me if I'm wrong):

For various reasons we can have out-of-range constants for arithmetic, 
logical or other operations.  Those out-of-range constants will 
typically be loaded into a register so that we can create valid insns.

LICM (and code motion in general) may hoist the constant register loads 
out of loops, which we generally consider a win (there's certainly cases 
where it is not though).  It's particularly helpful when the constant 
can be used by many instructions.

Global constant propagation may then try to replace uses of the constant 
by the constant itself.  Some of those propagations create valid insns, 
but insns with a higher cost than their prior form.  This is effectively 
undoing LICM.

The patch changes the constant propagator to check the rtx cost of the 
original form vs the propagated form and only propagates if the cost is 
the same or lower -- the obvious idea being to propagate the constant 
only when it saves us cycles.

Please correct me if I've got the overall summary incorrect.

There were several small issues raised that are probably worth a bit of 
further discussion.

Register pressure.  This patch can increase register pressure.  It 
happens if, prior to this patch the constant was propagated to all the 
use sites.  In that case the pseudo holding the constant is dead and 
gets eliminated.  With this patch we may decline to propagate the 
constant to the use site (due to cost) and as a result the pseudo 
remains live, thus increasing register pressure.

Based on Kugan's data, I don't see that as a major problem in practice. 
  Though Ilya might have specific cases for i686 PIC where it's a bigger 
concern.

Performance.  There wasn't a big win with this patch on either tested 
architecture -- which is no great surprise.  We're talking about very 
small cost differences, possibly differences that can be well hidden by 
modern pipelines.

General conerns about using rtx costing.  What Kugan is doing here is 
very similar to what's being done in other rtl passes WRT checking costs 
before making transformations.  So I don't see that as a significant 
reason to object to the patch.


WRT the patch itself.

The "const_p" variable is poorly named, though I can kindof see how you 
settled on it.  Maybe "check_rtx_costs" or something along those lines 
would be better.

The comment for the second hunk would probably be better as:

/* If TO is a constant, check the cost of the set after propagation
    to the cost of the set before the propagation.  If the cost is
    higher, then do not replace FROM with TO.  */


You should try to produce a testcase where this change shows a code 
generation improvement.    Given we're checking target costs, that test 
will naturally be target specific.  But please do try.

So with the two nits fixed and a testcase, I think this can go forward.

jeff

Re: [PR65768] Check rtx_cost when propagating constant

[Jeff Law]

On 05/13/2015 11:46 PM, Kugan wrote:
> ping?
>
> Thanks,
> Kugan
>
> On 15/04/15 17:53, Kugan wrote:
>> As mentioned in PR65768, ARM gcc generates suboptimal code for constant
>> Uses in loop. Part of the reason is cprop is undoing what loop invariant
>> code motion did.
>>
>> Zhenqiang posted a patch at to fix this based on rtx costs:
>> https://gcc.gnu.org/ml/gcc-patches/2014-06/msg01321.html
>>
>> I cleaned it up and bootstrapped, regression tested on x86_64-linux-gnu;
>> no new regressions. Is this OK for trunk?
>>
>> Thanks,
>> Kugan
>>
>> gcc/ChangeLog:
>>
>> 2015-04-15  Kugan Vivekanandarajah  <kuganv@linaro.org>
>> 	    Zhenqiang Chen  <zhenqiang.chen@linaro.org>
>>
>> 	PR target/65768
>> 	* cprop.c (try_replace_reg): Check cost of constants before propagating.
I should have also noted, fresh bootstrap & regression test is needed too.

jeff

Re: [PR65768] Check rtx_cost when propagating constant

[Kugan]

On 29/05/15 07:31, Jeff Law wrote:
> On 05/13/2015 11:46 PM, Kugan wrote:
>> ping?
>>
>> Thanks,
>> Kugan
>>
>> On 15/04/15 17:53, Kugan wrote:
>>> As mentioned in PR65768, ARM gcc generates suboptimal code for constant
>>> Uses in loop. Part of the reason is cprop is undoing what loop invariant
>>> code motion did.
>>>
>>> Zhenqiang posted a patch at to fix this based on rtx costs:
>>> https://gcc.gnu.org/ml/gcc-patches/2014-06/msg01321.html
>>>
>>> I cleaned it up and bootstrapped, regression tested on x86_64-linux-gnu;
>>> no new regressions. Is this OK for trunk?
>>>
>>> Thanks,
>>> Kugan
>>>
>>> gcc/ChangeLog:
>>>
>>> 2015-04-15  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>>         Zhenqiang Chen  <zhenqiang.chen@linaro.org>
>>>
>>>     PR target/65768
>>>     * cprop.c (try_replace_reg): Check cost of constants before
>>> propagating.
> I should have also noted, fresh bootstrap & regression test is needed too.

Thanks Jeff for the comments. I did a fresh bootstrap and regression
testing on x86_64-linux-gnu with no new regression. I will wait for you ACK.

Thanks,
Kugan

Re: [PR65768] Check rtx_cost when propagating constant

[Jeff Law]

On 05/29/2015 12:32 AM, Kugan wrote:
>>>>
>>>>      PR target/65768
>>>>      * cprop.c (try_replace_reg): Check cost of constants before
>>>> propagating.
>> I should have also noted, fresh bootstrap & regression test is needed too.
>
> Thanks Jeff for the comments. I did a fresh bootstrap and regression
> testing on x86_64-linux-gnu with no new regression. I will wait for you ACK.
Can you address the 3 issues in my prior message?  I'll include them 
here for clarity:

--

The "const_p" variable is poorly named, though I can kindof see how you 
settled on it.  Maybe "check_rtx_costs" or something along those lines 
would be better.

The comment for the second hunk would probably be better as:

/* If TO is a constant, check the cost of the set after propagation
    to the cost of the set before the propagation.  If the cost is
    higher, then do not replace FROM with TO.  */


You should try to produce a testcase where this change shows a code 
generation improvement.    Given we're checking target costs, that test 
will naturally be target specific.  But please do try.

So with the two nits fixed and a testcase, I think this can go forward.
--

Re: [PR65768] Check rtx_cost when propagating constant

[Kugan]

[-- Attachment #1: Type: text/plain, Size: 1870 bytes --]



On 30/05/15 14:54, Jeff Law wrote:
> On 05/29/2015 12:32 AM, Kugan wrote:
>>>>>
>>>>>      PR target/65768
>>>>>      * cprop.c (try_replace_reg): Check cost of constants before
>>>>> propagating.
>>> I should have also noted, fresh bootstrap & regression test is needed
>>> too.
>>
>> Thanks Jeff for the comments. I did a fresh bootstrap and regression
>> testing on x86_64-linux-gnu with no new regression. I will wait for
>> you ACK.
> Can you address the 3 issues in my prior message?  I'll include them
> here for clarity:
> 
> -- 
> 
> The "const_p" variable is poorly named, though I can kindof see how you
> settled on it.  Maybe "check_rtx_costs" or something along those lines
> would be better.
> 
> The comment for the second hunk would probably be better as:
> 
> /* If TO is a constant, check the cost of the set after propagation
>    to the cost of the set before the propagation.  If the cost is
>    higher, then do not replace FROM with TO.  */
> 
> 
> You should try to produce a testcase where this change shows a code
> generation improvement.    Given we're checking target costs, that test
> will naturally be target specific.  But please do try.
> 
> So with the two nits fixed and a testcase, I think this can go forward.
> -- 
> 

Thanks Jeff and apologies for missing your previous email. I have now
fixed the comments as you suggested and changed the PR target/65768
testcase such that it tests this case.

I will commit it if there is no objections to this.

Thanks,
Kugan

gcc/ChangeLog:

2015-06-01  Kugan Vivekanandarajah  <kuganv@linaro.org>
	    Zhenqiang Chen  <zhenqiang.chen@linaro.org>

	PR target/65768
	* cprop.c (try_replace_reg): Check cost of constants before propagating.


gcc/testsuite/ChangeLog:

2015-06-01  Kugan Vivekanandarajah  <kuganv@linaro.org>

	PR target/65768
	* gcc.target/arm/maskdata.c: Remove -fno-gcse.

[-- Attachment #2: cprop.txt --]
[-- Type: text/plain, Size: 1689 bytes --]

diff --git a/gcc/cprop.c b/gcc/cprop.c
index 57c44ef..94bb064 100644
--- a/gcc/cprop.c
+++ b/gcc/cprop.c
@@ -765,12 +765,37 @@ try_replace_reg (rtx from, rtx to, rtx_insn *insn)
   int success = 0;
   rtx set = single_set (insn);
 
+  bool check_rtx_costs = true;
+  bool speed = optimize_bb_for_speed_p (BLOCK_FOR_INSN (insn));
+  int old_cost = set ? set_rtx_cost (set, speed) : 0;
+
+  if ((note != 0
+      && REG_NOTE_KIND (note) == REG_EQUAL
+      && (GET_CODE (XEXP (note, 0)) == CONST
+	  || CONSTANT_P (XEXP (note, 0))))
+      || (set && CONSTANT_P (SET_SRC (set))))
+    check_rtx_costs = false;
+
   /* Usually we substitute easy stuff, so we won't copy everything.
      We however need to take care to not duplicate non-trivial CONST
      expressions.  */
   to = copy_rtx (to);
 
   validate_replace_src_group (from, to, insn);
+
+  /* If TO is a constant, check the cost of the set after propagation
+     to the cost of the set before the propagation.  If the cost is
+     higher, then do not replace FROM with TO.  */
+
+  if (check_rtx_costs
+      && CONSTANT_P (to)
+      && (set_rtx_cost (set, speed) > old_cost))
+    {
+      cancel_changes (0);
+      return false;
+    }
+
+
   if (num_changes_pending () && apply_change_group ())
     success = 1;
 
diff --git a/gcc/testsuite/gcc.target/arm/maskdata.c b/gcc/testsuite/gcc.target/arm/maskdata.c
index 6d6bb39..35d2f06 100644
--- a/gcc/testsuite/gcc.target/arm/maskdata.c
+++ b/gcc/testsuite/gcc.target/arm/maskdata.c
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options " -O2 -fno-gcse " }  */
+/* { dg-options " -O2" }  */
 /* { dg-require-effective-target arm_thumb2_ok } */
 
 #define MASK 0xff00ff

Re: [PR65768] Check rtx_cost when propagating constant

[Jeff Law]

On 05/31/2015 08:20 PM, Kugan wrote:
>
>
> On 30/05/15 14:54, Jeff Law wrote:
>> On 05/29/2015 12:32 AM, Kugan wrote:
>>>>>>
>>>>>>       PR target/65768
>>>>>>       * cprop.c (try_replace_reg): Check cost of constants before
>>>>>> propagating.
>>>> I should have also noted, fresh bootstrap & regression test is needed
>>>> too.
>>>
>>> Thanks Jeff for the comments. I did a fresh bootstrap and regression
>>> testing on x86_64-linux-gnu with no new regression. I will wait for
>>> you ACK.
>> Can you address the 3 issues in my prior message?  I'll include them
>> here for clarity:
>>
>> --
>>
>> The "const_p" variable is poorly named, though I can kindof see how you
>> settled on it.  Maybe "check_rtx_costs" or something along those lines
>> would be better.
>>
>> The comment for the second hunk would probably be better as:
>>
>> /* If TO is a constant, check the cost of the set after propagation
>>     to the cost of the set before the propagation.  If the cost is
>>     higher, then do not replace FROM with TO.  */
>>
>>
>> You should try to produce a testcase where this change shows a code
>> generation improvement.    Given we're checking target costs, that test
>> will naturally be target specific.  But please do try.
>>
>> So with the two nits fixed and a testcase, I think this can go forward.
>> --
>>
>
> Thanks Jeff and apologies for missing your previous email. I have now
> fixed the comments as you suggested and changed the PR target/65768
> testcase such that it tests this case.
>
> I will commit it if there is no objections to this.
No objections.  Thanks for your patience on this!

jeff