cfgrtl: Forbid forwarder blocks from having clobbers [PR125375]

In this testcase, jump2 was presented with:

 L1:
    set the return register
    do epilogue stuff
    goto L4

 L2:
    do the same epilogue stuff

 L3:
    clobber the return register
    goto L4

The question then is: is the L3 block a forwarder block?  It is a
forwarder block in the sense that a jump to L3 can be replaced with a
jump to L4.  But it isn't a forwarder block in the sense of a jump to L3
being equivalent to a jump to L4.  In particular, a jump to L4 cannot be
merged with a jump or fallthrough to L3 unless we can prove that the
clobber is valid for both paths.

In the testcase, L3 was marked as a forwarder block and so cross-jumping
created:

 L1:
    set the return register

 L2:
    do epilogue stuff

 L3:
    clobber the return register
    goto L4

The set of the return register was then inevitably deleted as dead.

The clobber in this case is of the return register.  But the same
principle/problem would apply to any clobber.  We can't introduce new
clobbers on a path without proving that the clobbered thing is dead.

This question arises due to an old quirk of active_insn_p that predates
CVS history:

  bool
  active_insn_p (const rtx_insn *insn)
  {
    return (CALL_P (insn) || JUMP_P (insn)
	    || JUMP_TABLE_DATA_P (insn) /* FIXME */
	    || (NONJUMP_INSN_P (insn)
		&& (! reload_completed
		    || (GET_CODE (PATTERN (insn)) != USE
			&& GET_CODE (PATTERN (insn)) != CLOBBER))));
  }

Thus a clobber is "active" before RA but not after it.  This means
that, according to flow_active_insn_p, a block with a clobber is not
a forwarder block before RA, but can be afterwards.

The "most optimal" solution would probably be to split the concept
of forwarder block into two, one that allows clobbers and one that
doesn't.  However, that would be difficult to retrofit at this stage
and isn't likely to be suitable for backporting.  This patch therefore
takes the more conservative approach of making flow_active_insn_p treat
clobbers in the same way after RA as it does before RA.

Some of this infrastructure is probably ripe for updating.  For example,
flow might have required explicit uses of the return register, but DF
should cope well enough without.  We should probably also check
whether the active_insn_p behaviour still makes sense.

gcc/
	PR rtl-optimization/125375
	* cfgrtl.cc (flow_active_insn_p): Return true for clobbers.

gcc/testsuite/
	* gcc.dg/pr125375.c: New test.

diff --git a/gcc/cfgrtl.cc b/gcc/cfgrtl.cc
index 0ec72f08fa873b978c82bea6d94ea5911c30f07b..474b84a08934ba6287b09490f91ba1dffcb1402a 100644 (file)
--- a/gcc/cfgrtl.cc
+++ b/gcc/cfgrtl.cc
@@ -558,8 +558,8 @@ update_bb_for_insn (basic_block bb)
 }
 
 \f
-/* Like active_insn_p, except keep the return value use or clobber around
-   even after reload.  */
+/* Return true if adding or removing instances of INSN might affect the
+   semantics of the RTL.  */
 
 static bool
 flow_active_insn_p (const rtx_insn *insn)
@@ -567,16 +567,29 @@ flow_active_insn_p (const rtx_insn *insn)
   if (active_insn_p (insn))
     return true;
 
-  /* A clobber of the function return value exists for buggy
-     programs that fail to return a value.  Its effect is to
-     keep the return value from being live across the entire
-     function.  If we allow it to be skipped, we introduce the
-     possibility for register lifetime confusion.
-     Similarly, keep a USE of the function return value, otherwise
-     the USE is dropped and we could fail to thread jump if USE
-     appears on some paths and not on others, see PR90257.  */
-  if ((GET_CODE (PATTERN (insn)) == CLOBBER
-       || GET_CODE (PATTERN (insn)) == USE)
+  /* We cannot add new clobbers to a path without first proving that
+     the clobbered thing is dead.
+
+     For example:
+
+       (code_label L1)
+       (clobber X)
+       (set (pc) (label_ref L2))
+
+     is a forwarder block in the sense that a jump to L1 can be replaced
+     with a jump to L2, although perhaps with some loss of dataflow precision.
+     However, any attempt to merge a jump to L1 with a jump to L2 would be an
+     asymmetric operation, in that the merged code must jump to L2 rather than
+     to L1.  Our current definition of "forwarder block" does not allow for
+     this distinction and so we need to take a conservatively correct
+     approach.  */
+  if (GET_CODE (PATTERN (insn)) == CLOBBER)
+    return true;
+
+  /* Keep a USE of the function return value, otherwise the USE is dropped and
+     we could fail to thread a jump if USE appears on some paths and not on
+     others, see PR90257.  */
+  if (GET_CODE (PATTERN (insn)) == USE
       && REG_P (XEXP (PATTERN (insn), 0))
       && REG_FUNCTION_VALUE_P (XEXP (PATTERN (insn), 0)))
     return true;

diff --git a/gcc/testsuite/gcc.dg/pr125375.c b/gcc/testsuite/gcc.dg/pr125375.c
new file mode 100644 (file)
index 0000000..512d237
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr125375.c
@@ -0,0 +1,29 @@
+/* { dg-do run } */
+/* { dg-options "-O2 -fno-inline -fno-compare-elim" } */
+
+int a, c = 1;
+short b = 1, e;
+volatile unsigned short d;
+static int f() {
+  unsigned g = 2;
+  if (c >= a)
+    if ((c || b) && b) {
+      unsigned h = c && d;
+      int i = e = d;
+      if (d)
+        i = d = 0;
+      g = c ^ g * i;
+      c = ~c;
+      b = b * (g | 9) & ((1 && a) - i);
+      h && i && d;
+      a = c ^ e ^ (g && a) * h;
+      d = e;
+      if (a)
+        return g;
+    }
+}
+int main() {
+  if (f() != 1)
+    __builtin_abort();
+  return 0;
+}