[llvm] 5db2735 - [gvn] Handle simply phi equivalence cases

[Philip Reames via llvm-commits]

Author: Philip Reames
Date: 2021-03-06T09:31:12-08:00
New Revision: 5db2735af91be3bfb2fcb06bf8e1ac5f7d1e4812

URL: https://github.com/llvm/llvm-project/commit/5db2735af91be3bfb2fcb06bf8e1ac5f7d1e4812
DIFF: https://github.com/llvm/llvm-project/commit/5db2735af91be3bfb2fcb06bf8e1ac5f7d1e4812.diff

LOG: [gvn] Handle simply phi equivalence cases

GVN basically doesn't handle phi nodes at all. This is for a reason - we can't value number their inputs since the predecessor blocks have probably not been visited yet.

However, it also creates a significant pass ordering problem. As it stands, instcombine and simplifycfg ends up implementing CSE of phi nodes. This means that for any series of CSE opportunities intermixed with phi nodes, we end up having to alternate instcombine/simplifycfg and gvn to make progress.

This patch handles the simplest case by simply preprocessing the phi instructions in a block, and CSEing them if they are syntactically identical. This turns out to be powerful enough to handle many cases in a single invocation of GVN since blocks which use the cse'd phi results are visited after the block containing the phi. If there's a CSE opportunity in one the phi predecessors required to recognize the phi CSE opportunity, that will require a second iteration on the function. (Still within a single run of gvn though.)

Compile time wise, this could go either way. On one hand, we're potentially causing GVN to iterate over the function more. On the other, we're cutting down on iterations between two passes and potentially shrinking the IR aggressively. So, a bit unclear what to expect.

Note that this does still rely on instcombine to canonicalize block order of the phis, but that's a one time transformation independent of the values incoming to the phi.

Differential Revision: https://reviews.llvm.org/D98080

Added: 
    

Modified: 
    llvm/lib/Transforms/Scalar/GVN.cpp
    llvm/test/Transforms/GVN/phi.ll

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/Scalar/GVN.cpp b/llvm/lib/Transforms/Scalar/GVN.cpp
index 30b947f6ef3d9..e8b8d477098ea 100644
--- a/llvm/lib/Transforms/Scalar/GVN.cpp
+++ b/llvm/lib/Transforms/Scalar/GVN.cpp
@@ -2372,6 +2372,12 @@ bool GVN::processBlock(BasicBlock *BB) {
   ReplaceOperandsWithMap.clear();
   bool ChangedFunction = false;
 
+  // Since we may not have visited the input blocks of the phis, we can't
+  // use our normal hash approach for phis.  Instead, simply look for
+  // obvious duplicates.  The first pass of GVN will tend to create
+  // identical phis, and the second or later passes can eliminate them.
+  ChangedFunction |= EliminateDuplicatePHINodes(BB);
+
   for (BasicBlock::iterator BI = BB->begin(), BE = BB->end();
        BI != BE;) {
     if (!ReplaceOperandsWithMap.empty())

diff  --git a/llvm/test/Transforms/GVN/phi.ll b/llvm/test/Transforms/GVN/phi.ll
index c6634ce91eb3d..b978281effbe2 100644
--- a/llvm/test/Transforms/GVN/phi.ll
+++ b/llvm/test/Transforms/GVN/phi.ll
@@ -11,9 +11,7 @@ define i64 @test1(i1 %c, i64 %a, i64 %b) {
 ; CHECK-NEXT:    br label [[MERGE]]
 ; CHECK:       merge:
 ; CHECK-NEXT:    [[PHI1:%.*]] = phi i64 [ [[A:%.*]], [[TAKEN]] ], [ [[B:%.*]], [[UNTAKEN]] ]
-; CHECK-NEXT:    [[PHI2:%.*]] = phi i64 [ [[A]], [[TAKEN]] ], [ [[B]], [[UNTAKEN]] ]
-; CHECK-NEXT:    [[RET:%.*]] = sub i64 [[PHI1]], [[PHI2]]
-; CHECK-NEXT:    ret i64 [[RET]]
+; CHECK-NEXT:    ret i64 0
 ;
   br i1 %c, label %taken, label %untaken
 taken:
@@ -69,9 +67,7 @@ define i64 @test3(i1 %c, i64 %a, i64 %b) {
 ; CHECK-NEXT:    br label [[MERGE]]
 ; CHECK:       merge:
 ; CHECK-NEXT:    [[PHI1:%.*]] = phi i64 [ [[ADD1]], [[TAKEN]] ], [ [[B:%.*]], [[UNTAKEN]] ]
-; CHECK-NEXT:    [[PHI2:%.*]] = phi i64 [ [[ADD1]], [[TAKEN]] ], [ [[B]], [[UNTAKEN]] ]
-; CHECK-NEXT:    [[RET:%.*]] = sub i64 [[PHI1]], [[PHI2]]
-; CHECK-NEXT:    ret i64 [[RET]]
+; CHECK-NEXT:    ret i64 0
 ;
   br i1 %c, label %taken, label %untaken
 taken:
@@ -96,19 +92,15 @@ define i64 @test4(i1 %c, i64 %a, i64 %b) {
 ; CHECK-NEXT:    br label [[MERGE]]
 ; CHECK:       merge:
 ; CHECK-NEXT:    [[PHI1:%.*]] = phi i64 [ [[A:%.*]], [[TAKEN]] ], [ [[B:%.*]], [[UNTAKEN]] ]
-; CHECK-NEXT:    [[PHI2:%.*]] = phi i64 [ [[A]], [[TAKEN]] ], [ [[B]], [[UNTAKEN]] ]
 ; CHECK-NEXT:    br i1 [[C]], label [[TAKEN2:%.*]], label [[UNTAKEN2:%.*]]
 ; CHECK:       taken2:
 ; CHECK-NEXT:    [[ADD1:%.*]] = add i64 [[PHI1]], 5
-; CHECK-NEXT:    [[ADD2:%.*]] = add i64 [[PHI2]], 5
 ; CHECK-NEXT:    br label [[MERGE2:%.*]]
 ; CHECK:       untaken2:
 ; CHECK-NEXT:    br label [[MERGE2]]
 ; CHECK:       merge2:
-; CHECK-NEXT:    [[PHI3:%.*]] = phi i64 [ [[ADD1]], [[TAKEN2]] ], [ [[PHI2]], [[UNTAKEN2]] ]
-; CHECK-NEXT:    [[PHI4:%.*]] = phi i64 [ [[ADD2]], [[TAKEN2]] ], [ [[PHI2]], [[UNTAKEN2]] ]
-; CHECK-NEXT:    [[RET:%.*]] = sub i64 [[PHI4]], [[PHI3]]
-; CHECK-NEXT:    ret i64 [[RET]]
+; CHECK-NEXT:    [[PHI3:%.*]] = phi i64 [ [[ADD1]], [[TAKEN2]] ], [ [[PHI1]], [[UNTAKEN2]] ]
+; CHECK-NEXT:    ret i64 0
 ;
   br i1 %c, label %taken, label %untaken
 taken: