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.
Thanks,
James
