diff mbox series

[RFA] PR target/111815: VAX: Only accept the index scaler as the RHS operand to ASHIFT

Message ID alpine.DEB.2.20.2310160141130.5892@tpp.orcam.me.uk
State Accepted
Headers
Series [RFA] PR target/111815: VAX: Only accept the index scaler as the RHS operand to ASHIFT |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Maciej W. Rozycki Oct. 16, 2023, 9:41 a.m. UTC 
  As from commit 9df1ba9a35b8 ("libbacktrace: support zstd decompression") 
GCC for the `vax-netbsdelf' target fails to complete building, with an 
ICE:

during RTL pass: final
.../libbacktrace/elf.c: In function 'elf_zstd_decompress':
.../libbacktrace/elf.c:5006:1: internal compiler error: in print_operand_address, at config/vax/vax.cc:514
 5006 | }
      | ^
0x1113df97 print_operand_address(_IO_FILE*, rtx_def*)
	.../gcc/config/vax/vax.cc:514
0x10c2489b default_print_operand_address(_IO_FILE*, machine_mode, rtx_def*)
	.../gcc/targhooks.cc:373
0x106ddd0b output_address(machine_mode, rtx_def*)
	.../gcc/final.cc:3648
0x106ddd0b output_asm_insn(char const*, rtx_def**)
	.../gcc/final.cc:3505
0x106e2143 output_asm_insn(char const*, rtx_def**)
	.../gcc/final.cc:3421
0x106e2143 final_scan_insn_1
	.../gcc/final.cc:2841
0x106e28e3 final_scan_insn(rtx_insn*, _IO_FILE*, int, int, int*)
	.../gcc/final.cc:2887
0x106e2bf7 final_1
	.../gcc/final.cc:1979
0x106e3c67 rest_of_handle_final
	.../gcc/final.cc:4240
0x106e3c67 execute
	.../gcc/final.cc:4318
Please submit a full bug report, with preprocessed source (by using -freport-bug).
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.

This is due to combine producing an invalid address RTX:

(plus:SI (ashift:SI (const_int 1 [0x1])
        (reg:QI 3 %r3 [1232]))
    (reg/v:SI 10 %r10 [orig:736 weight_mask ] [736]))

where the expression is ((1 << R3) + R10), which does not match a valid 
machine addressing mode.  Consequently `print_operand_address' chokes.

This can be reduced to the testcase included, where it triggers the same 
ICE in `p'.  Preincrements are required so that their results land in 
registers and consequently an indexed addressing mode is tried or 
otherwise doing operations piecemeal on stack-based function arguments 
as direct input operands turns out more profitable in terms of RTX costs 
and the ICE is avoided.

The ultimate cause has been commit c605a8bf9270 ("VAX: Accept ASHIFT in 
address expressions"), where a shift of an immediate value by a register 
has been mistakenly allowed as an index expression as if the shift 
operation was commutative such as multiplication is.  So with ASHIFT the 
scaler in an index expression has to be the right-hand operand, and the 
backend has to enforce that, whereas with MULT the scaler can be either 
operand.

Fix this by only accepting the index scaler as the RHS operand to 
ASHIFT.

	gcc/
	PR target/111815
	* config/vax/vax.cc (index_term_p): Only accept the index scaler 
	as the RHS operand to ASHIFT.

	gcc/testsuite/
	PR target/111815
	* gcc.dg/torture/pr111815.c: New test.
---
Hi,

 The testcase is generic enough I thought it wouldn't hurt to place it in 
a generic part of the testsuite, where it has been verified to pass with 
the `powerpc64le-linux-gnu', `riscv64-linux-gnu', and `vax-netbsdelf' 
targets.  I'm fine to move it to the VAX part of the testsuite though if 
there's disagreement as to my choice.  Otherwise OK to apply for this 
part?

 NB as a serious backend issue I mean to backport this change to active 
release branches as well.

  Maciej
---
 gcc/config/vax/vax.cc                   |    9 ++++++---
 gcc/testsuite/gcc.dg/torture/pr111815.c |   26 ++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 3 deletions(-)

gcc-vax-index-ashift-noncommutative.diff

Comments

Maciej W. Rozycki Oct. 30, 2023, 1:41 p.m. UTC | #1 
On Mon, 16 Oct 2023, Maciej W. Rozycki wrote:

>  The testcase is generic enough I thought it wouldn't hurt to place it in 
> a generic part of the testsuite, where it has been verified to pass with 
> the `powerpc64le-linux-gnu', `riscv64-linux-gnu', and `vax-netbsdelf' 
> targets.  I'm fine to move it to the VAX part of the testsuite though if 
> there's disagreement as to my choice.  Otherwise OK to apply for this 
> part?

 Ping for: 
<https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633112.html>.

  Maciej
Maciej W. Rozycki Nov. 13, 2023, 8:22 p.m. UTC | #2 
On Mon, 16 Oct 2023, Maciej W. Rozycki wrote:

>  The testcase is generic enough I thought it wouldn't hurt to place it in 
> a generic part of the testsuite, where it has been verified to pass with 
> the `powerpc64le-linux-gnu', `riscv64-linux-gnu', and `vax-netbsdelf' 
> targets.  I'm fine to move it to the VAX part of the testsuite though if 
> there's disagreement as to my choice.  Otherwise OK to apply for this 
> part?

 Ping for:
<https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633112.html>.

  Maciej
Jeff Law Nov. 13, 2023, 8:31 p.m. UTC | #3 
On 11/13/23 13:22, Maciej W. Rozycki wrote:
> On Mon, 16 Oct 2023, Maciej W. Rozycki wrote:
> 
>>   The testcase is generic enough I thought it wouldn't hurt to place it in
>> a generic part of the testsuite, where it has been verified to pass with
>> the `powerpc64le-linux-gnu', `riscv64-linux-gnu', and `vax-netbsdelf'
>> targets.  I'm fine to move it to the VAX part of the testsuite though if
>> there's disagreement as to my choice.  Otherwise OK to apply for this
>> part?
> 
>   Ping for:
> <https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633112.html>.
OK.  I'm fully supportive of generic tests :-)


Jeff
Maciej W. Rozycki Nov. 22, 2023, 2:29 a.m. UTC | #4 
On Mon, 13 Nov 2023, Jeff Law wrote:

> > >   The testcase is generic enough I thought it wouldn't hurt to place it in
> > > a generic part of the testsuite, where it has been verified to pass with
> > > the `powerpc64le-linux-gnu', `riscv64-linux-gnu', and `vax-netbsdelf'
> > > targets.  I'm fine to move it to the VAX part of the testsuite though if
> > > there's disagreement as to my choice.  Otherwise OK to apply for this
> > > part?
> > 
> >   Ping for:
> > <https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633112.html>.
> OK.  I'm fully supportive of generic tests :-)

 Thank you for the review.  I have now committed the fix and backported it 
to GCC 13/12/11.

  Maciej
diff mbox series

Patch

Index: gcc/gcc/config/vax/vax.cc
===================================================================
--- gcc.orig/gcc/config/vax/vax.cc
+++ gcc/gcc/config/vax/vax.cc
@@ -1831,7 +1831,9 @@  nonindexed_address_p (rtx x, bool strict
 }
 
 /* True if PROD is either a reg times size of mode MODE and MODE is less
-   than or equal 8 bytes, or just a reg if MODE is one byte.  */
+   than or equal 8 bytes, or just a reg if MODE is one byte.  For a MULT
+   RTX we accept its operands in either order, however ASHIFT is not
+   commutative, so in that case reg has to be the left operand.  */
 
 static bool
 index_term_p (rtx prod, machine_mode mode, bool strict)
@@ -1850,8 +1852,9 @@  index_term_p (rtx prod, machine_mode mod
   xfoo0 = XEXP (prod, 0);
   xfoo1 = XEXP (prod, 1);
 
-  if (CONST_INT_P (xfoo0)
-      && GET_MODE_SIZE (mode) == (log_p ? 1 << INTVAL (xfoo0) : INTVAL (xfoo0))
+  if (!log_p
+      && CONST_INT_P (xfoo0)
+      && GET_MODE_SIZE (mode) == INTVAL (xfoo0)
       && INDEX_REGISTER_P (xfoo1, strict))
     return true;
 
Index: gcc/gcc/testsuite/gcc.dg/torture/pr111815.c
===================================================================
--- /dev/null
+++ gcc/gcc/testsuite/gcc.dg/torture/pr111815.c
@@ -0,0 +1,26 @@ 
+/* { dg-do run } */
+
+char x[] = {
+   0,  1,  2,  3,  4,  5,  6,  7,  8,  9, 10, 11, 12, 13, 14, 15,
+  16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31,
+};
+
+__attribute__ ((noinline)) char *
+p (char *a, int o, int i)
+{
+  return a + ++o + (1 << ++i);
+}
+
+int
+main (void)
+{
+  if (*p (x, 0, 0) != 3)
+    return 1;
+  if (*p (x, 1, 2) != 10)
+    return 1;
+  if (*p (x, 2, 1) != 7)
+    return 1;
+  if (*p (x, 3, 3) != 20)
+    return 1;
+  return 0;
+}