Re: [PATCH AArch64]Fix ICE in cortex-a57 fma steering pass

["Bin.Cheng" <amker dot cheng at gmail dot com>]

On Fri, Jul 14, 2017 at 12:12 PM, James Greenhalgh
<james.greenhalgh@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.cheng@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.tkachov@arm.com>
>>
>>       PR target/81414
>>       * gcc.target/aarch64/pr81414.C: New.
>
>> From ef2bc842993210a4399205d83fa46435eec5d7cd Mon Sep 17 00:00:00 2001
>> From: Bin Cheng <binche01@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
>

Attachment: pr81414-20170714.txt
Description: Text document