[llvm-commits] Do more conditional propagation in GVN

[Duncan Sands]

Hi Chris,

>> PS: Here's an updated version of the patch to propagate equalities
>> between arbitrary instructions (as opposed to requiring a constant
>> on one side).
>
> Testcase?  Does this trigger on anything real?

yes, it fires a lot.  However most of the time the improvement to the code,
while pleasant, is not significant, which is why I was hesitating over it.
The balance was tipped by (1) measurements showing that the compile time
increase was in the noise; (2) requests that I commit it by various people
who wanted it; and (3) no sign that the patch increases register pressure,
if anything it seems to decrease it.  I committed a variant (with testcase)
here:

r151713 | baldrick | 2012-02-29 12:12:03 +0100 (Wed, 29 Feb 2012) | 3 lines

Have GVN also do condition propagation when the right-hand side is not
a constant.  This fixes PR1768.

Here's the diff:

Index: test/Transforms/GVN/condprop.ll
===================================================================
--- test/Transforms/GVN/condprop.ll	(revision 151712)
+++ test/Transforms/GVN/condprop.ll	(revision 151713)
@@ -175,3 +175,60 @@
  ; CHECK: ret i1 false
    ret i1 %cmp3
  }
+
+; PR1768
+; CHECK: @test9
+define i32 @test9(i32 %i, i32 %j) {
+  %cmp = icmp eq i32 %i, %j
+  br i1 %cmp, label %cond_true, label %ret
+
+cond_true:
+  %diff = sub i32 %i, %j
+  ret i32 %diff
+; CHECK: ret i32 0
+
+ret:
+  ret i32 5
+; CHECK: ret i32 5
+}
+
+; PR1768
+; CHECK: @test10
+define i32 @test10(i32 %j, i32 %i) {
+  %cmp = icmp eq i32 %i, %j
+  br i1 %cmp, label %cond_true, label %ret
+
+cond_true:
+  %diff = sub i32 %i, %j
+  ret i32 %diff
+; CHECK: ret i32 0
+
+ret:
+  ret i32 5
+; CHECK: ret i32 5
+}
+
+declare i32 @yogibar()
+
+; CHECK: @test11
+define i32 @test11(i32 %x) {
+  %v0 = call i32 @yogibar()
+  %v1 = call i32 @yogibar()
+  %cmp = icmp eq i32 %v0, %v1
+  br i1 %cmp, label %cond_true, label %next
+
+cond_true:
+  ret i32 %v1
+; CHECK: ret i32 %v0
+
+next:
+  %cmp2 = icmp eq i32 %x, %v0
+  br i1 %cmp2, label %cond_true2, label %next2
+
+cond_true2:
+  ret i32 %v0
+; CHECK: ret i32 %x
+
+next2:
+  ret i32 0
+}
Index: lib/Transforms/Scalar/GVN.cpp
===================================================================
--- lib/Transforms/Scalar/GVN.cpp	(revision 151712)
+++ lib/Transforms/Scalar/GVN.cpp	(revision 151713)
@@ -1972,21 +1972,30 @@
    if (isa<Constant>(LHS) && isa<Constant>(RHS))
      return false;

-  // Make sure that any constants are on the right-hand side.  In general the
-  // best results are obtained by placing the longest lived value on the RHS.
-  if (isa<Constant>(LHS))
+  // Prefer a constant on the right-hand side, or an Argument if no constants.
+  if (isa<Constant>(LHS) || (isa<Argument>(LHS) && !isa<Constant>(RHS)))
      std::swap(LHS, RHS);
+  assert((isa<Argument>(LHS) || isa<Instruction>(LHS)) && "Unexpected value!");

-  // If neither term is constant then bail out.  This is not for correctness,
-  // it's just that the non-constant case is much less useful: it occurs just
-  // as often as the constant case but handling it hardly ever results in an
-  // improvement.
-  if (!isa<Constant>(RHS))
-    return false;
+  // If there is no obvious reason to prefer the left-hand side over the right-
+  // hand side, ensure the longest lived term is on the right-hand side, so the
+  // shortest lived term will be replaced by the longest lived.  This tends to
+  // expose more simplifications.
+  uint32_t LVN = VN.lookup_or_add(LHS);
+  if ((isa<Argument>(LHS) && isa<Argument>(RHS)) ||
+      (isa<Instruction>(LHS) && isa<Instruction>(RHS))) {
+    // Move the 'oldest' value to the right-hand side, using the value number as
+    // a proxy for age.
+    uint32_t RVN = VN.lookup_or_add(RHS);
+    if (LVN < RVN) {
+      std::swap(LHS, RHS);
+      LVN = RVN;
+    }
+  }

    // If value numbering later deduces that an instruction in the scope is equal
    // to 'LHS' then ensure it will be turned into 'RHS'.
-  addToLeaderTable(VN.lookup_or_add(LHS), RHS, Root);
+  addToLeaderTable(LVN, RHS, Root);

    // Replace all occurrences of 'LHS' with 'RHS' everywhere in the scope.  As
    // LHS always has at least one use that is not dominated by Root, this will