[PATCH][ARM] PR 65489: Accept VSTRUCT constants in arm_legitimate_constant_p

[PATCH][ARM] PR 65489: Accept VSTRUCT constants in arm_legitimate_constant_p

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 1956 bytes --]

Hi all,

The ICE in the PR happens on arm during the hoist pass when the code
generates a SET rtx of the form:
(set (reg:OI) (const_int 0)). It checks whether const_int 0 is a
general_operand for OImode which involves asking the backend whether it's a
legitimate constant.

arm_legitimate_constant_p_1 explicitly rejects OImode constants as a result
of the fix for PR 46329
https://gcc.gnu.org/ml/gcc-patches/2011-04/msg00200.html

This results in gcse using an emit_insn of the SET rtx rather than trying to
do an emit_move_insn
which would have worked in this case.

From reading the thread I get the impression that the ICE there was caused
by reload and coproc_secondary_reload_class behaving weirdly. We're now
using LRA and
SECONDARY_INPUT_RELOAD_CLASS and SECONDARY_OUTPUT_RELOAD_CLASS which are the
only users of coproc_secondary_reload_class are no longer used.

I removed the restriction on constants for these large modes in
arm_legitimate_constant_p_1 and the testcases in PR 46329 all work fine.
Bootstrap, testing and building various vector workloads also didn't show
any problems.

Is this the right way to go for this?
If so, ok for trunk?

The ICE in this PR happens on a recently added testcase to
gcc.c-torture/execute/ and this patch fixes that, so no new testcase is
added. Jakub mentioned that maybe the testcase can be modified to manually
perform the lowering done in his patch to potentially trigger this on older
gcc versions, but I'm not sure how that would be done.
The problem is that something is generating OImode moves that I think can
only be done in the vector form of the testcase, so I'm not entirely
convinced that it's reproducible on 4.9 and 4.8.

Thanks,
Kyrill

Thanks,
Kyrill

2015-03-23  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

    PR target/65489
    * config/arm/arm.c (arm_legitimate_constant_p_1): Remove restriction
    on constants for NEON VSTRUCT modes.

[-- Attachment #2: arm-vstruct-const.patch --]
[-- Type: application/octet-stream, Size: 872 bytes --]

commit a4f39db799615199a0eb330d9254b543cb858198
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Thu Mar 19 17:37:41 2015 +0000

    [ARM] PR 65489: Accept VSTRUCT constants in arm_legitimate_constant_p

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 54102f6..356d5de 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -8416,14 +8416,8 @@ arm_tls_referenced_p (rtx x)
    When generating pic allow anything.  */
 
 static bool
-arm_legitimate_constant_p_1 (machine_mode mode, rtx x)
+arm_legitimate_constant_p_1 (machine_mode, rtx x)
 {
-  /* At present, we have no support for Neon structure constants, so forbid
-     them here.  It might be possible to handle simple cases like 0 and -1
-     in future.  */
-  if (TARGET_NEON && VALID_NEON_STRUCT_MODE (mode))
-    return false;
-
   return flag_pic || !label_mentioned_p (x);
 }
 

Re: [PATCH][ARM] PR 65489: Accept VSTRUCT constants in arm_legitimate_constant_p

[Kyrill Tkachov]

Ping.
https://gcc.gnu.org/ml/gcc-patches/2015-03/msg01196.html

Thanks,
Kyrill
On 23/03/15 16:15, Kyrill Tkachov wrote:
> Hi all,
>
> The ICE in the PR happens on arm during the hoist pass when the code
> generates a SET rtx of the form:
> (set (reg:OI) (const_int 0)). It checks whether const_int 0 is a
> general_operand for OImode which involves asking the backend whether it's a
> legitimate constant.
>
> arm_legitimate_constant_p_1 explicitly rejects OImode constants as a result
> of the fix for PR 46329
> https://gcc.gnu.org/ml/gcc-patches/2011-04/msg00200.html
>
> This results in gcse using an emit_insn of the SET rtx rather than trying to
> do an emit_move_insn
> which would have worked in this case.
>
>  From reading the thread I get the impression that the ICE there was caused
> by reload and coproc_secondary_reload_class behaving weirdly. We're now
> using LRA and
> SECONDARY_INPUT_RELOAD_CLASS and SECONDARY_OUTPUT_RELOAD_CLASS which are the
> only users of coproc_secondary_reload_class are no longer used.
>
> I removed the restriction on constants for these large modes in
> arm_legitimate_constant_p_1 and the testcases in PR 46329 all work fine.
> Bootstrap, testing and building various vector workloads also didn't show
> any problems.
>
> Is this the right way to go for this?
> If so, ok for trunk?
>
> The ICE in this PR happens on a recently added testcase to
> gcc.c-torture/execute/ and this patch fixes that, so no new testcase is
> added. Jakub mentioned that maybe the testcase can be modified to manually
> perform the lowering done in his patch to potentially trigger this on older
> gcc versions, but I'm not sure how that would be done.
> The problem is that something is generating OImode moves that I think can
> only be done in the vector form of the testcase, so I'm not entirely
> convinced that it's reproducible on 4.9 and 4.8.
>
> Thanks,
> Kyrill
>
> Thanks,
> Kyrill
>
> 2015-03-23  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>      PR target/65489
>      * config/arm/arm.c (arm_legitimate_constant_p_1): Remove restriction
>      on constants for NEON VSTRUCT modes.

Re: [PATCH][ARM] PR 65489: Accept VSTRUCT constants in arm_legitimate_constant_p

[Ramana Radhakrishnan]

On Mon, Mar 23, 2015 at 4:15 PM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
> Hi all,
>
> The ICE in the PR happens on arm during the hoist pass when the code
> generates a SET rtx of the form:
> (set (reg:OI) (const_int 0)). It checks whether const_int 0 is a
> general_operand for OImode which involves asking the backend whether it's a
> legitimate constant.
>
> arm_legitimate_constant_p_1 explicitly rejects OImode constants as a result
> of the fix for PR 46329
> https://gcc.gnu.org/ml/gcc-patches/2011-04/msg00200.html
>
> This results in gcse using an emit_insn of the SET rtx rather than trying to
> do an emit_move_insn
> which would have worked in this case.
>
> From reading the thread I get the impression that the ICE there was caused
> by reload and coproc_secondary_reload_class behaving weirdly. We're now
> using LRA and
> SECONDARY_INPUT_RELOAD_CLASS and SECONDARY_OUTPUT_RELOAD_CLASS which are the
> only users of coproc_secondary_reload_class are no longer used.
>
> I removed the restriction on constants for these large modes in
> arm_legitimate_constant_p_1 and the testcases in PR 46329 all work fine.
> Bootstrap, testing and building various vector workloads also didn't show
> any problems.
>
> Is this the right way to go for this?
> If so, ok for trunk?
>
> The ICE in this PR happens on a recently added testcase to
> gcc.c-torture/execute/ and this patch fixes that, so no new testcase is
> added. Jakub mentioned that maybe the testcase can be modified to manually
> perform the lowering done in his patch to potentially trigger this on older
> gcc versions, but I'm not sure how that would be done.
> The problem is that something is generating OImode moves that I think can
> only be done in the vector form of the testcase, so I'm not entirely
> convinced that it's reproducible on 4.9 and 4.8.


This is OK for trunk now that we are in wide-int mode on trunk for the ARM port.

Thanks,
Ramana

>
> Thanks,
> Kyrill
>
> Thanks,
> Kyrill
>
> 2015-03-23  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>     PR target/65489
>     * config/arm/arm.c (arm_legitimate_constant_p_1): Remove restriction
>     on constants for NEON VSTRUCT modes.

Re: [PATCH][ARM] PR 65489: Accept VSTRUCT constants in arm_legitimate_constant_p

[Kyrill Tkachov]

On 04/04/15 17:11, Ramana Radhakrishnan wrote:
> On Mon, Mar 23, 2015 at 4:15 PM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
>> Hi all,
>>
>> The ICE in the PR happens on arm during the hoist pass when the code
>> generates a SET rtx of the form:
>> (set (reg:OI) (const_int 0)). It checks whether const_int 0 is a
>> general_operand for OImode which involves asking the backend whether it's a
>> legitimate constant.
>>
>> arm_legitimate_constant_p_1 explicitly rejects OImode constants as a result
>> of the fix for PR 46329
>> https://gcc.gnu.org/ml/gcc-patches/2011-04/msg00200.html
>>
>> This results in gcse using an emit_insn of the SET rtx rather than trying to
>> do an emit_move_insn
>> which would have worked in this case.
>>
>>  From reading the thread I get the impression that the ICE there was caused
>> by reload and coproc_secondary_reload_class behaving weirdly. We're now
>> using LRA and
>> SECONDARY_INPUT_RELOAD_CLASS and SECONDARY_OUTPUT_RELOAD_CLASS which are the
>> only users of coproc_secondary_reload_class are no longer used.
>>
>> I removed the restriction on constants for these large modes in
>> arm_legitimate_constant_p_1 and the testcases in PR 46329 all work fine.
>> Bootstrap, testing and building various vector workloads also didn't show
>> any problems.
>>
>> Is this the right way to go for this?
>> If so, ok for trunk?
>>
>> The ICE in this PR happens on a recently added testcase to
>> gcc.c-torture/execute/ and this patch fixes that, so no new testcase is
>> added. Jakub mentioned that maybe the testcase can be modified to manually
>> perform the lowering done in his patch to potentially trigger this on older
>> gcc versions, but I'm not sure how that would be done.
>> The problem is that something is generating OImode moves that I think can
>> only be done in the vector form of the testcase, so I'm not entirely
>> convinced that it's reproducible on 4.9 and 4.8.
>
> This is OK for trunk now that we are in wide-int mode on trunk for the ARM port.

Thanks,
committed with r221892.

Kyrill

>
> Thanks,
> Ramana
>
>> Thanks,
>> Kyrill
>>
>> Thanks,
>> Kyrill
>>
>> 2015-03-23  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>      PR target/65489
>>      * config/arm/arm.c (arm_legitimate_constant_p_1): Remove restriction
>>      on constants for NEON VSTRUCT modes.