[PATCH] PR tree-opt/113673: Avoid load merging from potentially trapping additions.

[PATCH] PR tree-opt/113673: Avoid load merging from potentially trapping additions.

[Roger Sayle]

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


This patch fixes PR tree-optimization/113673, a P2 ice-on-valid regression
caused by load merging of (ptr[0]<<8)+ptr[1] when -ftrapv has been
specified.  When the operator is | or ^ this is safe, but for addition
of signed integer types, a trap may be generated/required, so merging this
idiom into a single non-trapping instruction is inappropriate, confusing
the compiler by transforming a basic block with an exception edge into one
without.  One fix is to be more selective for PLUS_EXPR than for
BIT_IOR_EXPR or BIT_XOR_EXPR in gimple-ssa-store-merging.cc's
find_bswap_or_nop_1 function.

An alternate solution might be to notice that in this idiom the addition
can't overflow, but that this detail wasn't apparent when exception edges
were added to the CFG.  In which case, it's safe to remove (or mark for
removal) the problematic exceptional edge.  Unfortunately updating the
CFG is a part of the compiler that I'm less familiar with.

This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
and make -k check, both with and without --target_board=unix{-m32}
with no new failures.  Ok for mainline?


2024-04-28  Roger Sayle  <roger@nextmovesoftware.com>

gcc/ChangeLog
        PR tree-optimization/113673
        * gimple-ssa-store-merging.cc (find_bswap_or_nop_1) <case
PLUS_EXPR>:
        Don't perform load merging if a signed addition may trap.

gcc/testsuite/ChangeLog
        PR tree-optimization/113673
        * g++.dg/pr113673.C: New test case.


Thanks in advance,
Roger
--


[-- Attachment #2: patchlm.txt --]
[-- Type: text/plain, Size: 1195 bytes --]

diff --git a/gcc/gimple-ssa-store-merging.cc b/gcc/gimple-ssa-store-merging.cc
index cb0cb5f..41a1066 100644
--- a/gcc/gimple-ssa-store-merging.cc
+++ b/gcc/gimple-ssa-store-merging.cc
@@ -776,9 +776,16 @@ find_bswap_or_nop_1 (gimple *stmt, struct symbolic_number *n, int limit)
 
       switch (code)
 	{
+	case PLUS_EXPR:
+	  /* Don't perform load merging if this addition can trap.  */
+	  if (cfun->can_throw_non_call_exceptions
+	      && INTEGRAL_TYPE_P (TREE_TYPE (rhs1))
+	      && TYPE_OVERFLOW_TRAPS (TREE_TYPE (rhs1)))
+	    return NULL;
+	  /* Fallthru.  */
+
 	case BIT_IOR_EXPR:
 	case BIT_XOR_EXPR:
-	case PLUS_EXPR:
 	  source_stmt1 = find_bswap_or_nop_1 (rhs1_stmt, &n1, limit - 1);
 
 	  if (!source_stmt1)
diff --git a/gcc/testsuite/g++.dg/pr113673.C b/gcc/testsuite/g++.dg/pr113673.C
new file mode 100644
index 0000000..1148977
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr113673.C
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-Os -fnon-call-exceptions -ftrapv" } */
+
+struct s { ~s(); };
+void
+h (unsigned char *data, int c)
+{
+  s a1;
+  while (c)
+    {
+      int m = *data++ << 8;
+      m += *data++;
+    }
+}

Re: [PATCH] PR tree-opt/113673: Avoid load merging from potentially trapping additions.

[Richard Biener]

On Sun, Apr 28, 2024 at 11:11 AM Roger Sayle <roger@nextmovesoftware.com> wrote:
>
>
> This patch fixes PR tree-optimization/113673, a P2 ice-on-valid regression
> caused by load merging of (ptr[0]<<8)+ptr[1] when -ftrapv has been
> specified.  When the operator is | or ^ this is safe, but for addition
> of signed integer types, a trap may be generated/required, so merging this
> idiom into a single non-trapping instruction is inappropriate, confusing
> the compiler by transforming a basic block with an exception edge into one
> without.  One fix is to be more selective for PLUS_EXPR than for
> BIT_IOR_EXPR or BIT_XOR_EXPR in gimple-ssa-store-merging.cc's
> find_bswap_or_nop_1 function.
>
> An alternate solution might be to notice that in this idiom the addition
> can't overflow, but that this detail wasn't apparent when exception edges
> were added to the CFG.  In which case, it's safe to remove (or mark for
> removal) the problematic exceptional edge.  Unfortunately updating the
> CFG is a part of the compiler that I'm less familiar with.
>
> This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
> and make -k check, both with and without --target_board=unix{-m32}
> with no new failures.  Ok for mainline?

Instead of

+       case PLUS_EXPR:
+         /* Don't perform load merging if this addition can trap.  */
+         if (cfun->can_throw_non_call_exceptions
+             && INTEGRAL_TYPE_P (TREE_TYPE (rhs1))
+             && TYPE_OVERFLOW_TRAPS (TREE_TYPE (rhs1)))
+           return NULL;

please check stmt_can_throw_internal (cfun, stmt) - the find_bswap_or_no_load
call in the function suffers from the same issue, so this should probably
be checked before that call even.

Thanks,
Richard.

>
> 2024-04-28  Roger Sayle  <roger@nextmovesoftware.com>
>
> gcc/ChangeLog
>         PR tree-optimization/113673
>         * gimple-ssa-store-merging.cc (find_bswap_or_nop_1) <case
> PLUS_EXPR>:
>         Don't perform load merging if a signed addition may trap.
>
> gcc/testsuite/ChangeLog
>         PR tree-optimization/113673
>         * g++.dg/pr113673.C: New test case.
>
>
> Thanks in advance,
> Roger
> --
>