[PATCH] D66977: [GVN] Propagate simple equalities from assumes within the tail of the block

[Florian Hahn via Phabricator via llvm-commits]

fhahn accepted this revision.
fhahn added a comment.
This revision is now accepted and ready to land.

LGTM, with a few nits/suggestions for comments. Also, it would be great if you could also add a test case with floats.



================
Comment at: lib/Transforms/Scalar/GVN.cpp:1390
 
+static bool hasUsersIn(Value *V, BasicBlock *BB) {
+  for (User *U : V->users())
----------------
could just be something like the snippet below, which might be short & explicit enough to have inline.
```
any_of(I.users(), [B](User *U) {
return isa<Instruction>(U) &&  cast<Instruction>(U)->getParent() == B;
});


================
Comment at: lib/Transforms/Scalar/GVN.cpp:1438
 
-  // If one of *cmp *eq operand is const, adding it to map will cover this:
+  // If we find an equality fact, canonicalize all dominated uses to one
+  // of the two values.  We heuristically choice the "oldest" of the two.
----------------
We only replace the uses that come after in the current BB, right? I think it would be better to say that, instead of talking about replacing all dominated uses



================
Comment at: lib/Transforms/Scalar/GVN.cpp:1439
+  // If we find an equality fact, canonicalize all dominated uses to one
+  // of the two values.  We heuristically choice the "oldest" of the two.
+  // 
----------------
It might be worth mentioning what we mean by oldest here (seen earlier during value numbering)


================
Comment at: lib/Transforms/Scalar/GVN.cpp:1450
+  // call void @llvm.assume(i1 %cmp)
+  // ret float %load ; will change it to ret float %v
   if (auto *CmpI = dyn_cast<CmpInst>(V)) {
----------------
Shouldn't that be %0? Did I miss where %v is coming from in the example?


================
Comment at: lib/Transforms/Scalar/GVN.cpp:1458
       Value *CmpRHS = CmpI->getOperand(1);
-      if (isa<Constant>(CmpLHS))
+      // Hueristically pick the better replacement -- the choice of heuristic
+      // isn't terribly important here, but the fact we canonicalize on some
----------------
 Typo  Hueristically


================
Comment at: lib/Transforms/Scalar/GVN.cpp:1479
+      // removed a trivial assume yet.
+      if (isa<Constant>(CmpLHS) && isa<Constant>(CmpRHS))
+        return Changed;
----------------
Could you add a test case for this case?


================
Comment at: lib/Transforms/Scalar/GVN.cpp:1482
+
+      LLVM_DEBUG(dbgs() << "Replacing dominated uses of "
+                 << *CmpLHS << " with "
----------------
Again I think mentioning that we only replacing uses in the current BB would be slightly clearer.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66977/new/

https://reviews.llvm.org/D66977