On Thu, Jul 20, 2017 at 09:53:41AM +0100, Bin.Cheng wrote:
> 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.
OK for trunk and backport.
Thanks,
James
+
