* [Patch, avr, PR85624] - Fix ICE when initializing 128-byte aligned array
@ 2018-07-18 13:11 Senthil Kumar Selvaraj
2018-08-06 10:53 ` Senthil Kumar Selvaraj
0 siblings, 1 reply; 4+ messages in thread
From: Senthil Kumar Selvaraj @ 2018-07-18 13:11 UTC (permalink / raw)
To: gcc-patches; +Cc: chertykov, gjl
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];
+}
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Patch, avr, PR85624] - Fix ICE when initializing 128-byte aligned array
2018-07-18 13:11 [Patch, avr, PR85624] - Fix ICE when initializing 128-byte aligned array Senthil Kumar Selvaraj
@ 2018-08-06 10:53 ` Senthil Kumar Selvaraj
2018-08-06 15:57 ` Jeff Law
0 siblings, 1 reply; 4+ messages in thread
From: Senthil Kumar Selvaraj @ 2018-08-06 10:53 UTC (permalink / raw)
To: gcc-patches; +Cc: chertykov
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];
> +}
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Patch, avr, PR85624] - Fix ICE when initializing 128-byte aligned array
2018-08-06 10:53 ` Senthil Kumar Selvaraj
@ 2018-08-06 15:57 ` Jeff Law
2018-08-07 6:38 ` Senthil Kumar Selvaraj
0 siblings, 1 reply; 4+ messages in thread
From: Jeff Law @ 2018-08-06 15:57 UTC (permalink / raw)
To: Senthil Kumar Selvaraj, gcc-patches; +Cc: chertykov
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Patch, avr, PR85624] - Fix ICE when initializing 128-byte aligned array
2018-08-06 15:57 ` Jeff Law
@ 2018-08-07 6:38 ` Senthil Kumar Selvaraj
0 siblings, 0 replies; 4+ messages in thread
From: Senthil Kumar Selvaraj @ 2018-08-07 6:38 UTC (permalink / raw)
To: Jeff Law, chertykov; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-08-07 6:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-18 13:11 [Patch, avr, PR85624] - Fix ICE when initializing 128-byte aligned array Senthil Kumar Selvaraj
2018-08-06 10:53 ` Senthil Kumar Selvaraj
2018-08-06 15:57 ` Jeff Law
2018-08-07 6:38 ` Senthil Kumar Selvaraj
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).