[Patch, avr, PR85624] - Fix ICE when initializing 128-byte aligned array

[Patch, avr, PR85624] - Fix ICE when initializing 128-byte aligned array

[Senthil Kumar Selvaraj]

Hi,

The below patch fixes an ICE for the avr target when the setmemhi
expander is involved.

The setmemhi expander generated RTL ends up as an unrecognized insn
if the alignment of the destination exceeds that of a QI
mode const_int (127), AND the number of bytes to set fits in a QI
mode const_int. The second condition prevents *clrmemhi from matching,
and *clrmemqi does not match because it expects operand 3 (the alignment
const_int rtx) to be QI mode, and a value of 128 or greater does not fit.
  
The patch fixes this by changing the *clrmemqi pattern to match a HI
mode const_int, and also adds a testcase.

Regression test showed no new failures, ok to commit to trunk?

Regards
Senthil

gcc/ChangeLog:
    
2018-07-18  Senthil Kumar Selvaraj  <senthilkumar.selvaraj@microchip.com>
    
    PR target/85624
    * config/avr/avr.md (*clrmemqi): Change mode of operands[2]
    from QI to HI.
    
gcc/testsuite/ChangeLog:
    
2018-07-18  Senthil Kumar Selvaraj  <senthilkumar.selvaraj@microchip.com>
    
    PR target/85624
    * gcc.target/avr/pr85624.c: New test.

diff --git gcc/config/avr/avr.md gcc/config/avr/avr.md
index e619e695418..644e3cfabc5 100644
--- gcc/config/avr/avr.md
+++ gcc/config/avr/avr.md
@@ -1095,7 +1095,7 @@
   [(set (mem:BLK (match_operand:HI 0 "register_operand" "e"))
         (const_int 0))
    (use (match_operand:QI 1 "register_operand" "r"))
-   (use (match_operand:QI 2 "const_int_operand" "n"))
+   (use (match_operand:HI 2 "const_int_operand" "n"))
    (clobber (match_scratch:HI 3 "=0"))
    (clobber (match_scratch:QI 4 "=&1"))]
   ""
diff --git gcc/testsuite/gcc.target/avr/pr85624.c gcc/testsuite/gcc.target/avr/pr85624.c
new file mode 100644
index 00000000000..ede2e80216a
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/pr85624.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-O3" } */
+
+/* This testcase exposes PR85624. An alignment directive with
+   a value greater than 127 on an array with dimensions that fit
+   QImode causes an 'unrecognizable insn' ICE. Turns out clrmemqi
+   did not match the pattern expanded by setmemhi, because it
+   assumed the alignment val will fit in a QI. */
+
+int foo() {
+  volatile int arr[3] __attribute__((aligned(128))) = {0};
+  return arr[2];
+}

Re: [Patch, avr, PR85624] - Fix ICE when initializing 128-byte aligned array

[Senthil Kumar Selvaraj]

Ping!

Regards
Senthil

Senthil Kumar Selvaraj writes:

> Hi,
>
> The below patch fixes an ICE for the avr target when the setmemhi
> expander is involved.
>
> The setmemhi expander generated RTL ends up as an unrecognized insn
> if the alignment of the destination exceeds that of a QI
> mode const_int (127), AND the number of bytes to set fits in a QI
> mode const_int. The second condition prevents *clrmemhi from matching,
> and *clrmemqi does not match because it expects operand 3 (the alignment
> const_int rtx) to be QI mode, and a value of 128 or greater does not fit.
>   
> The patch fixes this by changing the *clrmemqi pattern to match a HI
> mode const_int, and also adds a testcase.
>
> Regression test showed no new failures, ok to commit to trunk?
>
> Regards
> Senthil
>
> gcc/ChangeLog:
>     
> 2018-07-18  Senthil Kumar Selvaraj  <senthilkumar.selvaraj@microchip.com>
>     
>     PR target/85624
>     * config/avr/avr.md (*clrmemqi): Change mode of operands[2]
>     from QI to HI.
>     
> gcc/testsuite/ChangeLog:
>     
> 2018-07-18  Senthil Kumar Selvaraj  <senthilkumar.selvaraj@microchip.com>
>     
>     PR target/85624
>     * gcc.target/avr/pr85624.c: New test.
>
> diff --git gcc/config/avr/avr.md gcc/config/avr/avr.md
> index e619e695418..644e3cfabc5 100644
> --- gcc/config/avr/avr.md
> +++ gcc/config/avr/avr.md
> @@ -1095,7 +1095,7 @@
>    [(set (mem:BLK (match_operand:HI 0 "register_operand" "e"))
>          (const_int 0))
>     (use (match_operand:QI 1 "register_operand" "r"))
> -   (use (match_operand:QI 2 "const_int_operand" "n"))
> +   (use (match_operand:HI 2 "const_int_operand" "n"))
>     (clobber (match_scratch:HI 3 "=0"))
>     (clobber (match_scratch:QI 4 "=&1"))]
>    ""
> diff --git gcc/testsuite/gcc.target/avr/pr85624.c gcc/testsuite/gcc.target/avr/pr85624.c
> new file mode 100644
> index 00000000000..ede2e80216a
> --- /dev/null
> +++ gcc/testsuite/gcc.target/avr/pr85624.c
> @@ -0,0 +1,13 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O3" } */
> +
> +/* This testcase exposes PR85624. An alignment directive with
> +   a value greater than 127 on an array with dimensions that fit
> +   QImode causes an 'unrecognizable insn' ICE. Turns out clrmemqi
> +   did not match the pattern expanded by setmemhi, because it
> +   assumed the alignment val will fit in a QI. */
> +
> +int foo() {
> +  volatile int arr[3] __attribute__((aligned(128))) = {0};
> +  return arr[2];
> +}

Re: [Patch, avr, PR85624] - Fix ICE when initializing 128-byte aligned array

[Jeff Law]

On 08/06/2018 04:53 AM, Senthil Kumar Selvaraj wrote:
> Ping!
> 
> Regards
> Senthil
> 
> Senthil Kumar Selvaraj writes:
> 
>> Hi,
>>
>> The below patch fixes an ICE for the avr target when the setmemhi
>> expander is involved.
>>
>> The setmemhi expander generated RTL ends up as an unrecognized insn
>> if the alignment of the destination exceeds that of a QI
>> mode const_int (127), AND the number of bytes to set fits in a QI
>> mode const_int. The second condition prevents *clrmemhi from matching,
>> and *clrmemqi does not match because it expects operand 3 (the alignment
>> const_int rtx) to be QI mode, and a value of 128 or greater does not fit.
>>   
>> The patch fixes this by changing the *clrmemqi pattern to match a HI
>> mode const_int, and also adds a testcase.
>>
>> Regression test showed no new failures, ok to commit to trunk?
>>
>> Regards
>> Senthil
>>
>> gcc/ChangeLog:
>>     
>> 2018-07-18  Senthil Kumar Selvaraj  <senthilkumar.selvaraj@microchip.com>
>>     
>>     PR target/85624
>>     * config/avr/avr.md (*clrmemqi): Change mode of operands[2]
>>     from QI to HI.
>>     
>> gcc/testsuite/ChangeLog:
>>     
>> 2018-07-18  Senthil Kumar Selvaraj  <senthilkumar.selvaraj@microchip.com>
>>     
>>     PR target/85624
>>     * gcc.target/avr/pr85624.c: New test.
Given there's also pattern clrmemhi it feels like you're papering over a
bug elsewhere, possibly in the expanders.

Ultimately I'll leave the decision here to the AVR maintainers through.

jeff

Re: [Patch, avr, PR85624] - Fix ICE when initializing 128-byte aligned array

[Senthil Kumar Selvaraj]

Jeff Law writes:

> On 08/06/2018 04:53 AM, Senthil Kumar Selvaraj wrote:
>> Ping!
>> 
>> Regards
>> Senthil
>> 
>> Senthil Kumar Selvaraj writes:
>> 
>>> Hi,
>>>
>>> The below patch fixes an ICE for the avr target when the setmemhi
>>> expander is involved.
>>>
>>> The setmemhi expander generated RTL ends up as an unrecognized insn
>>> if the alignment of the destination exceeds that of a QI
>>> mode const_int (127), AND the number of bytes to set fits in a QI
>>> mode const_int. The second condition prevents *clrmemhi from matching,
>>> and *clrmemqi does not match because it expects operand 3 (the alignment
>>> const_int rtx) to be QI mode, and a value of 128 or greater does not fit.
>>>   
>>> The patch fixes this by changing the *clrmemqi pattern to match a HI
>>> mode const_int, and also adds a testcase.
>>>
>>> Regression test showed no new failures, ok to commit to trunk?
>>>
>>> Regards
>>> Senthil
>>>
>>> gcc/ChangeLog:
>>>     
>>> 2018-07-18  Senthil Kumar Selvaraj  <senthilkumar.selvaraj@microchip.com>
>>>     
>>>     PR target/85624
>>>     * config/avr/avr.md (*clrmemqi): Change mode of operands[2]
>>>     from QI to HI.
>>>     
>>> gcc/testsuite/ChangeLog:
>>>     
>>> 2018-07-18  Senthil Kumar Selvaraj  <senthilkumar.selvaraj@microchip.com>
>>>     
>>>     PR target/85624
>>>     * gcc.target/avr/pr85624.c: New test.
> Given there's also pattern clrmemhi it feels like you're papering over a
> bug elsewhere, possibly in the expanders.

clrmemhi and clrmemqi differ on the mode of the register used to hold
the number of bytes to clear. The setmemhi expander picks a QI or HI
mode reg depending on the width of the size operand, and the
clrmem{qi,hi} match on that.

The operand whose mode I modified represents the alignment of the
destination, and isn't actually used in the assembler template.

Regards
Senthil
>
> Ultimately I'll leave the decision here to the AVR maintainers through.
>
> jeff