[tree-ssa-address] Use simplify_gen_binary in gen_addr_rtx

Message ID 586FC9BA.7020608@foss.arm.com
State New
Headers show

Commit Message

Kyrill Tkachov Jan. 6, 2017, 4:45 p.m.
On 05/01/17 12:09, Kyrill Tkachov wrote:
>

> On 05/01/17 12:01, Richard Biener wrote:

>> On Wed, Jan 4, 2017 at 4:07 PM, Kyrill Tkachov

>> <kyrylo.tkachov@foss.arm.com> wrote:

>>> On 04/01/17 14:19, Richard Biener wrote:

>>>> On Wed, Dec 21, 2016 at 10:40 AM, Kyrill Tkachov

>>>> <kyrylo.tkachov@foss.arm.com> wrote:

>>>>> On 20/12/16 17:30, Richard Biener wrote:

>>>>>> On December 20, 2016 5:01:19 PM GMT+01:00, Kyrill Tkachov

>>>>>> <kyrylo.tkachov@foss.arm.com> wrote:

>>>>>>> Hi all,

>>>>>>>

>>>>>>> The testcase in this patch generates bogus assembly for arm with -O1

>>>>>>> -mfloat-abi=soft:

>>>>>>>           strd    r4, [#0, r3]

>>>>>>>

>>>>>>> This is due to non-canonical RTL being generated during expansion:

>>>>>>> (set (mem:DI (plus:SI (const_int 0 [0])

>>>>>>>      (reg/f:SI 153)) [0 MEM[symbol: a, index: _26, offset: 0B]+0 S8

>>>>>>> A64])

>>>>>>>            (reg:DI 154))

>>>>>>>

>>>>>>> Note the (plus (const_int 0) (reg)). This is being generated in

>>>>>>> gen_addr_rtx in tree-ssa-address.c

>>>>>>> where it creates an explicit PLUS rtx through gen_rtx_PLUS, which

>>>>>>> doesn't try to canonicalise its arguments

>>>>>>> or simplify. The correct thing to do is to use simplify_gen_binary that

>>>>>>> will handle all this properly.

>>>>>> But it has to match up the validity check which passes down exactly the

>>>>>> same RTL(?)  Or does this stem from propagation simplifying a MEM after

>>>>>> IVOPTs?

>>>>>

>>>>> You mean TARGET_LEGITIMATE_ADDRESS_P? Yes, it gets passed on to that, but

>>>>> the arm implementation of that

>>>>> doesn't try to handle non-canonical RTL (plus (const0_rtx) (reg) is not

>>>>> canonical).

>>>>> Or do you mean some other check?

>>>> Ok, now looking at the actual patch and the code in question.  For your

>>>> testcase

>>>> it happens that symbol == const0_rtx?  In this case please amend the

>>>> if (symbol) check like we do for the base, thus

>>>>

>>>>      if (symbol && symbol != const0_rtx)

>>>

>>> No, symbol is not const0_rtx (it's just a symbol_ref).

>>> index is const0_rtx and so gets assigned to *addr at the beginning of the

>>> function.

>>> base and step are NULL_RTX.

>>> So at the time of the check:

>>>         if (*addr)

>>>           *addr = gen_rtx_PLUS (address_mode, *addr, act_elem);

>>>         else

>>>           *addr = act_elem;

>>>

>>> *addr is const0_rtx. Do you want to amend that check to:

>>>      if (*addr && *addr != const0_rtx) ?

>> Hmm, I guess given the if (step) in if (index) *addr can end up being

>> a not simplified mult.  So instead do

>>

>>     if (index && index != const0_rtx)

>

> That works, I'll test a patch for this.

>


Here it is. Bootstrapped and tested on arm-none-linux-gnueabihf and aarch64-none-linux-gnu.
Ok?

Thanks,
Kyrill

2017-01-06  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * tree-ssa-address.c (gen_addr_rtx): Don't handle index if it
     is const0_rtx.

2017-01-06  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * gcc.dg/20161219.c: New test.

>>> I haven't looked where the const0_rtx index comes from, so I don't know if

>>> it

>>> could have other CONST_INT values that may cause trouble.

>> Usually this happens when constant folding / propagation happens after

>> IVOPTs generates the TARGET_MEM_REF.  We do have some canonicalization

>> foldings for TMR, see maybe_fold_tmr, but that should have made index NULL

>> if it was constant...  So maybe we fail to fold a stmt at some point.

>>

>> Btw, I fail to see the bogus asm with my arm-non-eabi cross with -O

>> -mfloat-abi=soft

>> so I can't tell how the TMR arrives at RTL expansion.

>

> You'll also want to specify -marm (this doesn't trigger on Thumb) and perhaps -march=armv7-a.

>

> Thanks,

> Kyrill

>

>> Richard.

>>

>>> Kyrill

>>>

>>>

>>>> Richard.

>>>>

>>>>> Thanks,

>>>>> Kyrill

>>>>>

>>>>>

>>>>>>> I didn't change the other gen_rtx_PLUS calls in this function as their

>>>>>>> results is later used in XEXP operations

>>>>>>> that seem to rely on a PLUS expression being explicitly produced, but

>>>>>>> this particular call doesn't, so it's okay

>>>>>>> to change it. With this patch the sane assembly is generated:

>>>>>>>            strd    r4, [r3]

>>>>>>>

>>>>>>> Bootstrapped and tested on arm-none-linux-gnueabihf, x86_64,

>>>>>>> aarch64-none-linux-gnu.

>>>>>>>

>>>>>>> Ok for trunk?

>>>>>>>

>>>>>>> Thanks,

>>>>>>> Kyrill

>>>>>>>

>>>>>>> 2016-12-20  Kyrylo Tkachov <kyrylo.tkachov@arm.com>

>>>>>>>

>>>>>>>       * tree-ssa-address.c (gen_addr_rtx): Use simplify_gen_binary to

>>>>>>> add

>>>>>>>        *addr to act_elem.

>>>>>>>

>>>>>>> 2016-12-20  Kyrylo Tkachov <kyrylo.tkachov@arm.com>

>>>>>>>

>>>>>>>        * gcc.dg/20161219.c: New test.

>>>>>>

>

Comments

Richard Biener Jan. 6, 2017, 5:55 p.m. | #1
On January 6, 2017 5:45:46 PM GMT+01:00, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote:
>

>On 05/01/17 12:09, Kyrill Tkachov wrote:

>>

>> On 05/01/17 12:01, Richard Biener wrote:

>>> On Wed, Jan 4, 2017 at 4:07 PM, Kyrill Tkachov

>>> <kyrylo.tkachov@foss.arm.com> wrote:

>>>> On 04/01/17 14:19, Richard Biener wrote:

>>>>> On Wed, Dec 21, 2016 at 10:40 AM, Kyrill Tkachov

>>>>> <kyrylo.tkachov@foss.arm.com> wrote:

>>>>>> On 20/12/16 17:30, Richard Biener wrote:

>>>>>>> On December 20, 2016 5:01:19 PM GMT+01:00, Kyrill Tkachov

>>>>>>> <kyrylo.tkachov@foss.arm.com> wrote:

>>>>>>>> Hi all,

>>>>>>>>

>>>>>>>> The testcase in this patch generates bogus assembly for arm

>with -O1

>>>>>>>> -mfloat-abi=soft:

>>>>>>>>           strd    r4, [#0, r3]

>>>>>>>>

>>>>>>>> This is due to non-canonical RTL being generated during

>expansion:

>>>>>>>> (set (mem:DI (plus:SI (const_int 0 [0])

>>>>>>>>      (reg/f:SI 153)) [0 MEM[symbol: a, index: _26, offset:

>0B]+0 S8

>>>>>>>> A64])

>>>>>>>>            (reg:DI 154))

>>>>>>>>

>>>>>>>> Note the (plus (const_int 0) (reg)). This is being generated in

>>>>>>>> gen_addr_rtx in tree-ssa-address.c

>>>>>>>> where it creates an explicit PLUS rtx through gen_rtx_PLUS,

>which

>>>>>>>> doesn't try to canonicalise its arguments

>>>>>>>> or simplify. The correct thing to do is to use

>simplify_gen_binary that

>>>>>>>> will handle all this properly.

>>>>>>> But it has to match up the validity check which passes down

>exactly the

>>>>>>> same RTL(?)  Or does this stem from propagation simplifying a

>MEM after

>>>>>>> IVOPTs?

>>>>>>

>>>>>> You mean TARGET_LEGITIMATE_ADDRESS_P? Yes, it gets passed on to

>that, but

>>>>>> the arm implementation of that

>>>>>> doesn't try to handle non-canonical RTL (plus (const0_rtx) (reg)

>is not

>>>>>> canonical).

>>>>>> Or do you mean some other check?

>>>>> Ok, now looking at the actual patch and the code in question.  For

>your

>>>>> testcase

>>>>> it happens that symbol == const0_rtx?  In this case please amend

>the

>>>>> if (symbol) check like we do for the base, thus

>>>>>

>>>>>      if (symbol && symbol != const0_rtx)

>>>>

>>>> No, symbol is not const0_rtx (it's just a symbol_ref).

>>>> index is const0_rtx and so gets assigned to *addr at the beginning

>of the

>>>> function.

>>>> base and step are NULL_RTX.

>>>> So at the time of the check:

>>>>         if (*addr)

>>>>           *addr = gen_rtx_PLUS (address_mode, *addr, act_elem);

>>>>         else

>>>>           *addr = act_elem;

>>>>

>>>> *addr is const0_rtx. Do you want to amend that check to:

>>>>      if (*addr && *addr != const0_rtx) ?

>>> Hmm, I guess given the if (step) in if (index) *addr can end up

>being

>>> a not simplified mult.  So instead do

>>>

>>>     if (index && index != const0_rtx)

>>

>> That works, I'll test a patch for this.

>>

>

>Here it is. Bootstrapped and tested on arm-none-linux-gnueabihf and

>aarch64-none-linux-gnu.

>Ok?


OK.

Richard.

>Thanks,

>Kyrill

>

>2017-01-06  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

>

>     * tree-ssa-address.c (gen_addr_rtx): Don't handle index if it

>     is const0_rtx.

>

>2017-01-06  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

>

>     * gcc.dg/20161219.c: New test.

>

>>>> I haven't looked where the const0_rtx index comes from, so I don't

>know if

>>>> it

>>>> could have other CONST_INT values that may cause trouble.

>>> Usually this happens when constant folding / propagation happens

>after

>>> IVOPTs generates the TARGET_MEM_REF.  We do have some

>canonicalization

>>> foldings for TMR, see maybe_fold_tmr, but that should have made

>index NULL

>>> if it was constant...  So maybe we fail to fold a stmt at some

>point.

>>>

>>> Btw, I fail to see the bogus asm with my arm-non-eabi cross with -O

>>> -mfloat-abi=soft

>>> so I can't tell how the TMR arrives at RTL expansion.

>>

>> You'll also want to specify -marm (this doesn't trigger on Thumb) and

>perhaps -march=armv7-a.

>>

>> Thanks,

>> Kyrill

>>

>>> Richard.

>>>

>>>> Kyrill

>>>>

>>>>

>>>>> Richard.

>>>>>

>>>>>> Thanks,

>>>>>> Kyrill

>>>>>>

>>>>>>

>>>>>>>> I didn't change the other gen_rtx_PLUS calls in this function

>as their

>>>>>>>> results is later used in XEXP operations

>>>>>>>> that seem to rely on a PLUS expression being explicitly

>produced, but

>>>>>>>> this particular call doesn't, so it's okay

>>>>>>>> to change it. With this patch the sane assembly is generated:

>>>>>>>>            strd    r4, [r3]

>>>>>>>>

>>>>>>>> Bootstrapped and tested on arm-none-linux-gnueabihf, x86_64,

>>>>>>>> aarch64-none-linux-gnu.

>>>>>>>>

>>>>>>>> Ok for trunk?

>>>>>>>>

>>>>>>>> Thanks,

>>>>>>>> Kyrill

>>>>>>>>

>>>>>>>> 2016-12-20  Kyrylo Tkachov <kyrylo.tkachov@arm.com>

>>>>>>>>

>>>>>>>>       * tree-ssa-address.c (gen_addr_rtx): Use

>simplify_gen_binary to

>>>>>>>> add

>>>>>>>>        *addr to act_elem.

>>>>>>>>

>>>>>>>> 2016-12-20  Kyrylo Tkachov <kyrylo.tkachov@arm.com>

>>>>>>>>

>>>>>>>>        * gcc.dg/20161219.c: New test.

>>>>>>>

>>

Patch hide | download patch | download mbox

diff --git a/gcc/testsuite/gcc.dg/20161219.c b/gcc/testsuite/gcc.dg/20161219.c
new file mode 100644
index 0000000000000000000000000000000000000000..93ea8d2364d9ab54704a84e6c0bff0427df82db8
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/20161219.c
@@ -0,0 +1,30 @@ 
+/* { dg-do assemble } */
+/* { dg-options "-O1 -w" } */
+
+static long long a[9];
+int b, c, d, e, g;
+
+static int
+fn1 (int *p1)
+{
+  b = 1;
+  for (; b >= 0; b--)
+    {
+      d = 0;
+      for (;; d++)
+	{
+	  e && (a[d] = 0);
+	  if (*p1)
+	    break;
+	  c = (int) a;
+	}
+    }
+  return 0;
+}
+
+int
+main ()
+{
+  int f = fn1 ((int *) f);
+  return f;
+}
diff --git a/gcc/tree-ssa-address.c b/gcc/tree-ssa-address.c
index 3e3cad150b64e813509e079f9ea91d65806e414a..8d46a3e67337dd7639d0b17ca888f50009d65b93 100644
--- a/gcc/tree-ssa-address.c
+++ b/gcc/tree-ssa-address.c
@@ -115,7 +115,7 @@  gen_addr_rtx (machine_mode address_mode,
   if (offset_p)
     *offset_p = NULL;
 
-  if (index)
+  if (index && index != const0_rtx)
     {
       act_elem = index;
       if (step)