This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][tree-ssa-address] Use simplify_gen_binary in gen_addr_rtx
- From: Kyrill Tkachov <kyrylo dot tkachov at foss dot arm dot com>
- To: Richard Biener <richard dot guenther at gmail dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Fri, 06 Jan 2017 16:45:46 +0000
- Subject: Re: [PATCH][tree-ssa-address] Use simplify_gen_binary in gen_addr_rtx
- Authentication-results: sourceware.org; auth=none
- References: <585955CF.3010501@foss.arm.com> <1F1B16CA-0488-4666-9EA8-2B68A55C66C5@gmail.com> <585A4DFF.8080202@foss.arm.com> <CAFiYyc1Y+hbk9mr+=h0AYgMuCMq9YoKzqWFVzZXw62BMRvV7Zg@mail.gmail.com> <586D0FC9.2000403@foss.arm.com> <CAFiYyc2jsYLBmJGbedtsAOaGaxFmaKFWCu9gJ-qWw=Unyz3Xaw@mail.gmail.com> <586E3760.1060008@foss.arm.com>
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.
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)