diff mbox series

PR middle-end/105874: Use EXPAND_MEMORY to fix ada bootstrap.

Message ID 07f101d87b38$8c2b42a0$a481c7e0$@nextmovesoftware.com
State New
Headers show
Series PR middle-end/105874: Use EXPAND_MEMORY to fix ada bootstrap. | expand

Commit Message

Roger Sayle June 8, 2022, 1:06 p.m. UTC 
Many thanks to Tamar Christina for filing PR middle-end/105874 indicating
that SPECcpu 2017's Leela is failing on x86_64 due to a miscompilation
of FastBoard::is_eye.  This function is much smaller and easier to work
with than my previous hunt for the cause of the Ada bootstrap failures
due to miscompilation somewhere in GCC (or one of the 131 places that
the problematic form of optimization triggers during an ada bootstrap).

It turns out the source of the miscompilation introduced by my recent
patch is the distinction (during RTL expansion) of l-values and r-values.
According to the documentation above expand_modifier, EXPAND_MEMORY
should be used for lvalues (when a memory is required), and EXPAND_NORMAL
for rvalues when a constant is permissible.  In what I'd like to consider
a latent bug, the recursive call to expand_expr_real on line 11188 of
expr.cc, in the case handling ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF
and ARRARY_RANGE_REF was passing EXPAND_NORMAL when it really required
(the semantics of) EXPAND_MEMORY.  All the time that VAR_DECLs were
being returned as memory this was fine, but as soon as we're able to
optimize sort arrays into immediate constants, bad things happen.

In the test case from Leela, we notice that the array s_eyemask
always has DImode constant value { 4, 64 }, which is useful as
an rvalue, but not when we need to index it as an lvalue, as in
s_eyemask[color].  This also explains why everything being accepted
by immediate_const_ctor_p (during an ada bootstrap) looks reasonable,
what's incorrect is that we don't know how these structs/arrays are
to be used.

The fix is to ensure that we call expand_expr with EXPAND_MEMORY
when processing the VAR_DECL's returned by get_inner_reference.

This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
and make -k check (with no new failures), but also with
--enable-languages="ada" where it allows the bootstrap to finish,
and with no unexpected failures in the acats and gnat testsuites.
Ok for mainline?


2022-06-08  Roger Sayle  <roger@nextmovesoftware.com>

gcc/ChangeLog
        PR middle-end/105874
        * gcc/expr.cc (expand_expr_real_1) <normal_inner_ref>:  New local
        variable tem_modifier for calculating the expand_modifier enum to
        use for expanding tem.  If tem is a VAR_DECL, use EXPAND_MEMORY.

gcc/testsuite/ChangeLog
        PR middle-end/105874
        * g++.dg/opt/pr105874.C: New test case.


Sorry again for the inconvenience/breakage.
Roger
--

Comments

Eric Botcazou June 8, 2022, 2:13 p.m. UTC | #1 
> The fix is to ensure that we call expand_expr with EXPAND_MEMORY
> when processing the VAR_DECL's returned by get_inner_reference.
> 
> This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
> and make -k check (with no new failures), but also with
> --enable-languages="ada" where it allows the bootstrap to finish,
> and with no unexpected failures in the acats and gnat testsuites.
> Ok for mainline?

Yes, thanks (modulo the nit in the ChangeLog).

> 2022-06-08  Roger Sayle  <roger@nextmovesoftware.com>
> 
> gcc/ChangeLog
>         PR middle-end/105874
>         * gcc/expr.cc (expand_expr_real_1) <normal_inner_ref>:  New local
>         variable tem_modifier for calculating the expand_modifier enum to
>         use for expanding tem.  If tem is a VAR_DECL, use EXPAND_MEMORY.

gcc/ prefix to be stripped
Richard Biener June 13, 2022, 10:42 a.m. UTC | #2 
On Wed, Jun 8, 2022 at 4:14 PM Eric Botcazou via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> > The fix is to ensure that we call expand_expr with EXPAND_MEMORY
> > when processing the VAR_DECL's returned by get_inner_reference.
> >
> > This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
> > and make -k check (with no new failures), but also with
> > --enable-languages="ada" where it allows the bootstrap to finish,
> > and with no unexpected failures in the acats and gnat testsuites.
> > Ok for mainline?
>
> Yes, thanks (modulo the nit in the ChangeLog).

+       /* If tem is a VAR_DECL, we need a memory reference.  */
+       enum expand_modifier tem_modifier = modifier;
+       if (tem_modifier == EXPAND_SUM)
+         tem_modifier = EXPAND_NORMAL;
+       if (TREE_CODE (tem) == VAR_DECL)
+         tem_modifier = EXPAND_MEMORY;

that changes EXPAND_WRITE to EXPAND_MEMORY for VAR_DECL
for example - what's 'modifier' in the problematic case?  I do not understand
how 'VAR_DECL' is special here btw. - it seems to be a condition making
sure the new optimization doesn't trigger rather than a condition that will
always require memory?

Richard.

> > 2022-06-08  Roger Sayle  <roger@nextmovesoftware.com>
> >
> > gcc/ChangeLog
> >         PR middle-end/105874
> >         * gcc/expr.cc (expand_expr_real_1) <normal_inner_ref>:  New local
> >         variable tem_modifier for calculating the expand_modifier enum to
> >         use for expanding tem.  If tem is a VAR_DECL, use EXPAND_MEMORY.
>
> gcc/ prefix to be stripped
>
> --
> Eric Botcazou
>
>
Eric Botcazou June 13, 2022, 11:29 a.m. UTC | #3 
> +       /* If tem is a VAR_DECL, we need a memory reference.  */
> +       enum expand_modifier tem_modifier = modifier;
> +       if (tem_modifier == EXPAND_SUM)
> +         tem_modifier = EXPAND_NORMAL;
> +       if (TREE_CODE (tem) == VAR_DECL)
> +         tem_modifier = EXPAND_MEMORY;
> 
> that changes EXPAND_WRITE to EXPAND_MEMORY for VAR_DECL
> for example - what's 'modifier' in the problematic case?

My understanding is that it was EXPAND_NORMAL.

> I do not understand how 'VAR_DECL' is special here btw. - it seems to be a
> condition making sure the new optimization doesn't trigger rather than a
> condition that will always require memory?

It may indeed be too big a hammer.

Roger, would it be sufficient to use EXPAND_MEMORY only when must_force_mem 
computed a few lines below if true?
diff mbox series

Patch

diff --git a/gcc/expr.cc b/gcc/expr.cc
index fb062dc..a013650 100644
--- a/gcc/expr.cc
+++ b/gcc/expr.cc
@@ -11183,6 +11183,13 @@  expand_expr_real_1 (tree exp, rtx target, machine_mode tmode,
 	   infinitely recurse.  */
 	gcc_assert (tem != exp);
 
+	/* If tem is a VAR_DECL, we need a memory reference.  */
+	enum expand_modifier tem_modifier = modifier;
+	if (tem_modifier == EXPAND_SUM)
+	  tem_modifier = EXPAND_NORMAL;
+	if (TREE_CODE (tem) == VAR_DECL)
+	  tem_modifier = EXPAND_MEMORY;
+
 	/* If TEM's type is a union of variable size, pass TARGET to the inner
 	   computation, since it will need a temporary and TARGET is known
 	   to have to do.  This occurs in unchecked conversion in Ada.  */
@@ -11194,9 +11201,7 @@  expand_expr_real_1 (tree exp, rtx target, machine_mode tmode,
 				   != INTEGER_CST)
 			       && modifier != EXPAND_STACK_PARM
 			       ? target : NULL_RTX),
-			      VOIDmode,
-			      modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier,
-			      NULL, true);
+			      VOIDmode, tem_modifier, NULL, true);
 
 	/* If the field has a mode, we want to access it in the
 	   field's mode, not the computed mode.
diff --git a/gcc/testsuite/g++.dg/opt/pr105874.C b/gcc/testsuite/g++.dg/opt/pr105874.C
new file mode 100644
index 0000000..58699a6
--- /dev/null
+++ b/gcc/testsuite/g++.dg/opt/pr105874.C
@@ -0,0 +1,30 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -std=c++11" } */
+#include <array>
+
+static constexpr int NBR_SHIFT = 4;
+
+static constexpr int MAXBOARDSIZE = 25;
+
+static constexpr int MAXSQ = ((MAXBOARDSIZE + 2) * (MAXBOARDSIZE + 2));
+
+enum square_t : char {
+        BLACK = 0, WHITE = 1, EMPTY = 2, INVAL = 3
+    };
+
+const std::array<int, 2> s_eyemask = {
+    4 * (1 << (NBR_SHIFT * BLACK)),
+    4 * (1 << (NBR_SHIFT * WHITE))
+};
+
+/* counts of neighboring stones */
+std::array<unsigned short, MAXSQ> m_neighbours;
+
+int is_eye(const int color, const int i) {
+    /* check for 4 neighbors of the same color */
+    int ownsurrounded = (m_neighbours[i] & s_eyemask[color]);
+
+    return ownsurrounded;
+}
+
+/* { dg-final { scan-assembler "s_eyemask" } } */