[PATCH] D32990: [NewGVN] Take in account incoming edges computing congruent PhiExpression(s)

[Davide Italiano via Phabricator via llvm-commits]

davide created this revision.
Herald added a subscriber: Prazek.

I may be completely off on this one, please bear with me :)
The way we currently define congruency for two PHIExpression(s) is:

1. The operands to the phi functions are congruent
2. The PHIs are defined in the same Basic Block.

The attached testcase contains the following:

  patatino:
    %meh = phi i16 [ %0, %winky ], [ %conv1, %tinky ]
    %banana = phi i16 [ %0, %tinky ], [ %conv1, %winky ]
    br label %end

Looking at the NewGVN debug we believe that `%meh` and `%banana` are congruent. I don't think they are, in fact this causes a miscompile (you can run the following on the testcase).

  Processing instruction   %meh = phi i16 [ %0, %winky ], [ %conv1, %tinky ]
  Created new congruence class for   %meh = phi i16 [ %0, %winky ], [ %conv1, %tinky ] using expression { ExpressionTypePhi, opcode = 53, operands = {[0] = i16 %0  [1] = i16 %conv1  } bb = 0x486b1e0} at 10 and leader   %meh = phi i16 [ %0, %winky ], [ %conv1, %tinky ]
  New class 10 for expression { ExpressionTypePhi, opcode = 53, operands = {[0] = i16 %0  [1] = i16 %conv1  } bb = 0x486b1e0}
  Processing instruction   %banana = phi i16 [ %0, %tinky ], [ %conv1, %winky ]
  New class 10 for expression { ExpressionTypePhi, opcode = 53, operands = {[0] = i16 %0  [1] = i16 %conv1  } bb = 0x486b1e0}



  [davide at cupiditate bin]$ ./opt -newgvn phi.ll | ./lli
  65535
  65535
  [davide at cupiditate bin]$ ./opt -gvn phi.ll | ./lli
  0
  65535

I'm under the impression the problem is that we don't take in consideration the incoming edges for congruency. This patch makes sure we only consider congruent phis if the incoming BBs are the same. 
If we shouldn't, I'll appreciate suggestions on how to fix alternatively :)


https://reviews.llvm.org/D32990

Files:
  include/llvm/Transforms/Scalar/GVNExpression.h
  lib/Transforms/Scalar/NewGVN.cpp
  test/Transforms/NewGVN/pr32952.ll


Index: test/Transforms/NewGVN/pr32952.ll
===================================================================
--- /dev/null
+++ test/Transforms/NewGVN/pr32952.ll
@@ -0,0 +1,42 @@
+; PR32952: Don't erroneously consider congruent two phi nodes which
+; have the same arguments but different incoming edges.
+; RUN: opt -newgvn -S %s | FileCheck %s
+
+ at a = common global i16 0, align 2
+ at .str = private unnamed_addr constant [4 x i8] c"%d\0A\00", align 1
+
+define i32 @tinkywinky() {
+entry:
+  %0 = load i16, i16* @a, align 2
+  %conv = sext i16 %0 to i32
+  %neg = xor i32 %conv, -1
+  %conv1 = trunc i32 %neg to i16
+  %conv3 = zext i16 %conv1 to i32
+  %cmp = icmp slt i32 %conv, %conv3
+  br i1 %cmp, label %tinky, label %winky
+
+tinky:
+  store i16 2, i16* @a, align 2
+  br label %patatino
+
+winky:
+  br label %patatino
+
+patatino:
+; CHECK: %meh = phi i16 [ %0, %winky ], [ %conv1, %tinky ]
+; CHECK: %banana = phi i16 [ %0, %tinky ], [ %conv1, %winky ]
+  %meh = phi i16 [ %0, %winky ], [ %conv1, %tinky ]
+  %banana = phi i16 [ %0, %tinky ], [ %conv1, %winky ]
+  br label %end
+
+end:
+; CHECK: %promoted = zext i16 %banana to i32
+; CHECK: %other = zext i16 %meh to i32
+  %promoted = zext i16 %banana to i32
+  %other = zext i16 %meh to i32
+  %first = tail call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i64 0, i64 0), i32 %promoted)
+  %second = tail call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i64 0, i64 0), i32 %other)
+  ret i32 0
+}
+
+declare i32 @printf(i8*, ...)
Index: lib/Transforms/Scalar/NewGVN.cpp
===================================================================
--- lib/Transforms/Scalar/NewGVN.cpp
+++ lib/Transforms/Scalar/NewGVN.cpp
@@ -713,8 +713,11 @@
                                            bool &AllConstant) {
   BasicBlock *PHIBlock = I->getParent();
   auto *PN = cast<PHINode>(I);
-  auto *E =
-      new (ExpressionAllocator) PHIExpression(PN->getNumOperands(), PHIBlock);
+  std::vector<BasicBlock *> IBBs;
+  for (BasicBlock *BB : PN->blocks())
+    IBBs.push_back(BB);
+  auto *E = new (ExpressionAllocator)
+      PHIExpression(PN->getNumOperands(), PHIBlock, IBBs);
 
   E->allocateOperands(ArgRecycler, ExpressionAllocator);
   E->setType(I->getType());
Index: include/llvm/Transforms/Scalar/GVNExpression.h
===================================================================
--- include/llvm/Transforms/Scalar/GVNExpression.h
+++ include/llvm/Transforms/Scalar/GVNExpression.h
@@ -478,10 +478,12 @@
 class PHIExpression final : public BasicExpression {
 private:
   BasicBlock *BB;
+  std::vector<BasicBlock *> IncomingBBs;
 
 public:
-  PHIExpression(unsigned NumOperands, BasicBlock *B)
-      : BasicExpression(NumOperands, ET_Phi), BB(B) {}
+  PHIExpression(unsigned NumOperands, BasicBlock *B,
+                std::vector<BasicBlock *> IBBs)
+      : BasicExpression(NumOperands, ET_Phi), BB(B), IncomingBBs(IBBs) {}
   PHIExpression() = delete;
   PHIExpression(const PHIExpression &) = delete;
   PHIExpression &operator=(const PHIExpression &) = delete;
@@ -495,7 +497,13 @@
     if (!this->BasicExpression::equals(Other))
       return false;
     const PHIExpression &OE = cast<PHIExpression>(Other);
-    return BB == OE.BB;
+    if (BB != OE.BB)
+      return false;
+    for (unsigned I = 0; I != getNumOperands(); ++I) {
+      if (IncomingBBs[I] != OE.IncomingBBs[I])
+        return false;
+    }
+    return true;
   }
 
   hash_code getHashValue() const override {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D32990.98244.patch
Type: text/x-patch
Size: 3515 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170509/5a6e638d/attachment.bin>