Here's the consolidated patch what I've just installed.

ifcombine field merge: do not follow a second conversion [PR118046]

The testcase shows that conversions that would impact negatively the
ifcombine field merging implementation won't always have been
optimized out by the time we reach ifcombine.

There's probably room to support multiple conversions with extra
logic, but this workaround should avoid codegen errors until that
logic is figured out.


for  gcc/ChangeLog

        PR tree-optimization/118046
        * gimple-fold.cc (decode_field_reference): Don't follow more
        than one conversion.

for  gcc/testsuite/ChangeLog

        PR tree-optimization/118046
        * gcc.dg/field-merge-14.c: New.
---
 gcc/gimple-fold.cc                    |   41 ++++++++++++++-------------------
 gcc/testsuite/gcc.dg/field-merge-14.c |   26 +++++++++++++++++++++
 2 files changed, 44 insertions(+), 23 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/field-merge-14.c

diff --git a/gcc/gimple-fold.cc b/gcc/gimple-fold.cc
index 46b4874c4885c..6554b65572d01 100644
--- a/gcc/gimple-fold.cc
+++ b/gcc/gimple-fold.cc
@@ -7550,17 +7550,18 @@ decode_field_reference (tree *pexp, HOST_WIDE_INT 
*pbitsize,
   if (! INTEGRAL_TYPE_P (TREE_TYPE (exp)))
     return NULL_TREE;
 
-  /* Drop casts, only save the outermost type.  We need not worry about
-     narrowing then widening casts, or vice-versa, for those that are not
-     essential for the compare have already been optimized out at this
-     point.  */
-  if (gimple_convert_def_p (exp, res_ops))
-    {
-      if (!outer_type)
-       {
-         outer_type = TREE_TYPE (exp);
-         loc[0] = gimple_location (SSA_NAME_DEF_STMT (exp));
-       }
+  /* Drop casts, saving only the outermost type, effectively used in
+     the compare.  We can deal with at most one conversion, and it may
+     appear at various points in the chain of recognized preparation
+     statements.  Earlier optimizers will often have already dropped
+     unneeded extensions, but they may survive, as in PR118046.  ???
+     Can we do better and allow multiple conversions, perhaps taking
+     note of the narrowest intermediate type, sign extensions and
+     whatnot?  */
+  if (!outer_type && gimple_convert_def_p (exp, res_ops))
+    {
+      outer_type = TREE_TYPE (exp);
+      loc[0] = gimple_location (SSA_NAME_DEF_STMT (exp));
       exp = res_ops[0];
     }
 
@@ -7597,13 +7598,10 @@ decode_field_reference (tree *pexp, HOST_WIDE_INT 
*pbitsize,
     }
 
   /* Another chance to drop conversions.  */
-  if (gimple_convert_def_p (exp, res_ops))
+  if (!outer_type && gimple_convert_def_p (exp, res_ops))
     {
-      if (!outer_type)
-       {
-         outer_type = TREE_TYPE (exp);
-         loc[0] = gimple_location (SSA_NAME_DEF_STMT (exp));
-       }
+      outer_type = TREE_TYPE (exp);
+      loc[0] = gimple_location (SSA_NAME_DEF_STMT (exp));
       exp = res_ops[0];
     }
 
@@ -7623,13 +7621,10 @@ decode_field_reference (tree *pexp, HOST_WIDE_INT 
*pbitsize,
   /* Yet another chance to drop conversions.  This one is allowed to
      match a converting load, subsuming the load identification block
      below.  */
-  if (gimple_convert_def_p (exp, res_ops, load))
+  if (!outer_type && gimple_convert_def_p (exp, res_ops, load))
     {
-      if (!outer_type)
-       {
-         outer_type = TREE_TYPE (exp);
-         loc[0] = gimple_location (SSA_NAME_DEF_STMT (exp));
-       }
+      outer_type = TREE_TYPE (exp);
+      loc[0] = gimple_location (SSA_NAME_DEF_STMT (exp));
       if (*load)
        loc[3] = gimple_location (*load);
       exp = res_ops[0];
diff --git a/gcc/testsuite/gcc.dg/field-merge-14.c 
b/gcc/testsuite/gcc.dg/field-merge-14.c
new file mode 100644
index 0000000000000..91d84cfebf196
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/field-merge-14.c
@@ -0,0 +1,26 @@
+/* { dg-do run } */
+/* { dg-options "-O -fdump-tree-ifcombine-details" } */
+
+/* Check that we don't get confused by multiple conversions.  */
+
+__attribute__((noipa))
+int f(int *a,int *d)
+{
+  signed char b = *a;
+  int c = b;
+  *d = c; // This store is important even if otherwise unused
+  if (c < 0 && (b&1))
+    return 1;
+  return 0;
+}
+
+int main()
+{
+  unsigned char t = 0x81;
+  int x = t, y;
+  int tt = f(&x, &y);
+  if (!tt)
+    __builtin_abort();
+}
+
+/* { dg-final { scan-tree-dump-not "optimizing" "ifcombine" } } */


-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

Reply via email to