On Fri, Jul 14, 2017 at 12:12 PM, James Greenhalgh
<james.greenha...@arm.com> wrote:
> On Wed, Jul 12, 2017 at 03:15:04PM +0000, Bin Cheng wrote:
>> Hi,
>> After change @236817, AArch64 backend could avoid unnecessary conversion
>> instructions for register between different modes now. As a result, GCC
>> could initialize register in larger mode and use it later in smaller mode.
>> such def-use chain is not supported by current regrename.c analyzer, as
>> described by its comment:
>>
>> /* Process the insn, determining its effect on the def-use
>> chains and live hard registers. We perform the following
>> steps with the register references in the insn, simulating
>> its effect:
>> .......
>> We cannot deal with situations where we track a reg in one mode
>> and see a reference in another mode; these will cause the chain
>> to be marked unrenamable or even cause us to abort the entire
>> basic block. */
>>
>> In this case, regrename.c analyzer doesn't create chain for the use of the
>> register. OTOH, cortex-a57-fma-steering.c has below code:
>>
>> @@ -973,10 +973,14 @@ func_fma_steering::analyze ()
>> break;
>> }
>>
>> - /* We didn't find a chain with a def for this instruction. */
>> - gcc_assert (i < dest_op_info->n_chains);
>> -
>> - this->analyze_fma_fmul_insn (forest, chain, head);
>>
>> It assumes by gcc_assert that a chain must be found for dest register of
>> fmul/fmac instructions. According to above analysis, this is not always true
>> if the dest reg is reused from one of its source register.
>>
>> This patch fixes the issue by skipping such instructions if no du chain is
>> found. Bootstrap and test on AArch64/cortex-a57. Is it OK? If it's fine, I
>> would also need to backport it to 7/6 branches.
>
> This looks OK, but feels a bit like a workaround. Do you have a PR open
> for the missed optimisation caused by the deficiency in regrename?
>
> If so, it would be good to add that PR number to your comment in this
> function.
>
> For now, and for the backport, this will be fine, but your (Kyrill's) testcase
> has confused me (maybe too reduced from the original form) and doesn't
> match the bug here.
>
>> 2017-07-12 Bin Cheng <bin.ch...@arm.com>
>>
>> PR target/81414
>> * config/aarch64/cortex-a57-fma-steering.c (analyze): Skip fmul/fmac
>> instructions if no du chain is found.
>>
>> gcc/testsuite/ChangeLog
>> 2017-07-12 Kyrylo Tkachov <kyrylo.tkac...@arm.com>
>>
>> PR target/81414
>> * gcc.target/aarch64/pr81414.C: New.
>
>> From ef2bc842993210a4399205d83fa46435eec5d7cd Mon Sep 17 00:00:00 2001
>> From: Bin Cheng <binch...@e108451-lin.cambridge.arm.com>
>> Date: Wed, 12 Jul 2017 15:16:53 +0100
>> Subject: [PATCH] tmp
>>
>> ---
>> gcc/config/aarch64/cortex-a57-fma-steering.c | 12 ++++++++----
>> gcc/testsuite/gcc.target/aarch64/pr81414.C | 10 ++++++++++
>> 2 files changed, 18 insertions(+), 4 deletions(-)
>> create mode 100644 gcc/testsuite/gcc.target/aarch64/pr81414.C
>>
>> diff --git a/gcc/config/aarch64/cortex-a57-fma-steering.c
>> b/gcc/config/aarch64/cortex-a57-fma-steering.c
>> index 1bf804b..b2ee398 100644
>> --- a/gcc/config/aarch64/cortex-a57-fma-steering.c
>> +++ b/gcc/config/aarch64/cortex-a57-fma-steering.c
>> @@ -973,10 +973,14 @@ func_fma_steering::analyze ()
>> break;
>> }
>>
>> - /* We didn't find a chain with a def for this instruction. */
>> - gcc_assert (i < dest_op_info->n_chains);
>> -
>> - this->analyze_fma_fmul_insn (forest, chain, head);
>> + /* Due to implementation of regrename, dest register can slip away
>> + from regrename's analysis. As a result, there is no chain for
>> + the destination register of insn. We simply skip the insn even
>> + it is a fmul/fmac instruction. This case can happen when the
>> + dest register is also a source register of insn and the source
>> + reg is setup in larger mode than this insn. */
>> + if (i < dest_op_info->n_chains)
>> + this->analyze_fma_fmul_insn (forest, chain, head);
>> }
>> }
>> free (bb_dfs_preorder);
>> diff --git a/gcc/testsuite/gcc.target/aarch64/pr81414.C
>> b/gcc/testsuite/gcc.target/aarch64/pr81414.C
>> new file mode 100644
>> index 0000000..13666a3
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/aarch64/pr81414.C
>> @@ -0,0 +1,10 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2 -mcpu=cortex-a57" } */
>> +
>> +typedef __Float32x2_t float32x2_t;
>> +__inline float32x2_t vdup_n_f32(float) {}
>> +
>> +float32x2_t vfma_lane_f32(float32x2_t __a, float32x2_t __b) {
>> + int __lane;
>> + return __builtin_aarch64_fmav2sf(__b, vdup_n_f32(__lane), __a);
>> +}
>
> I don't see a mode-change here. This looks like it would have a bad def/use
> chain because of the unitialised __lane, rather than the issue here.
>
> In practice, your patch probably fixes two related issues - one where the
> def/use chain can't be formed because the register is used unitialised
> (this testcase) and one where the def/use chain can't be tracked through a
> mode-change.
>
> While fixing both of these in one go is fine, it would be good to update
> the comment to make clear this is what is happening, and to add a second
> testcase covering the mode-change issue.
>
> OK with the comment changed and a second testcase.
Hi,
This is the new patch. Updated comment describes all cases that
regrename could fail to build du chain. Thanks Kyrylo for creating
the new test, while I failed to create test showing the different
modes case. Is it OK? Also it would need to be backported to 7/6
branches.
Thanks,
bin
>
> Thanks,
> James
>
diff --git a/gcc/config/aarch64/cortex-a57-fma-steering.c
b/gcc/config/aarch64/cortex-a57-fma-steering.c
index 6d90acd..fa8c56a 100644
--- a/gcc/config/aarch64/cortex-a57-fma-steering.c
+++ b/gcc/config/aarch64/cortex-a57-fma-steering.c
@@ -973,10 +973,17 @@ func_fma_steering::analyze ()
break;
}
- /* We didn't find a chain with a def for this instruction. */
- gcc_assert (i < dest_op_info->n_chains);
-
- this->analyze_fma_fmul_insn (forest, chain, head);
+ /* Due to implementation of regrename, dest register can slip away
+ from regrename's analysis. As a result, there is no chain for
+ the destination register of insn. We simply skip the insn even
+ it is a fmul/fmac instruction. This can happen when the dest
+ register is also a source register of insn and one of the below
+ conditions is satisfied:
+ 1) the source reg is setup in larger mode than this insn;
+ 2) the source reg is uninitialized;
+ 3) the source reg is passed in as parameter. */
+ if (i < dest_op_info->n_chains)
+ this->analyze_fma_fmul_insn (forest, chain, head);
}
}
free (bb_dfs_preorder);
diff --git a/gcc/testsuite/gcc.target/aarch64/pr81414.C
b/gcc/testsuite/gcc.target/aarch64/pr81414.C
new file mode 100644
index 0000000..53dfc7c
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr81414.C
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -mcpu=cortex-a57" } */
+
+typedef __Float32x2_t float32x2_t;
+float32x2_t
+foo1 (float32x2_t __a, float32x2_t __b, float32x2_t __c) {
+ return __b * __c + __a;
+}
+
