[PATCH] D144927: [GVNHoist] don't hoist callbr users into the callbr's block

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/test/Transforms/GVNHoist/hoist-call.ll:139
+entry:
+  callbr void asm "", "=*m,!i"(ptr elementtype(i32) @x)
+  to label %a [label %b]
----------------
nickdesaulniers wrote:
> efriedma wrote:
> > I just verified that if you mark this callbr "readnone", hoisting happens.  Please adjust the testcase to demonstrate that hoisting.
> I can do that...but...
> 
> If you add `readnone` to the above test case `@foo3` (not `@foo4` which you were referring to), we also hoist, which looks wrong to me...
Specifically:
```
-  callbr void asm "", "=*m,!i"(ptr elementtype(i32) @x)
+  callbr void asm "", "=*m,!i"(ptr elementtype(i32) @x) readnone
```
Using `-passes='print<memoryssa>` we can see that this simply eliminates the initial MemoryDef on the callbr.

---

I think we might need to do something like: https://reviews.llvm.org/D145400
```
diff --git a/llvm/lib/Transforms/Scalar/GVNHoist.cpp b/llvm/lib/Transforms/Scalar/GVNHoist.cpp
index b23182db20f0..d7d1fefcde44 100644
--- a/llvm/lib/Transforms/Scalar/GVNHoist.cpp
+++ b/llvm/lib/Transforms/Scalar/GVNHoist.cpp
@@ -818,6 +818,13 @@ void GVNHoist::checkSafety(CHIArgs C, BasicBlock *BB, GVNHoist::InsKind K,
     // the use above the def.
     if (is_contained(T->users(), Insn))
       continue;
+    if (const auto *CB = dyn_cast<CallBase>(T)) {
+      if (CB->isInlineAsm()) {
+        // For each operand
+        //   if we alias with insn
+        //     skip
+      }
+    }
     if (K == InsKind::Scalar) {
       if (safeToHoistScalar(BB, Insn->getParent(), NumBBsOnAllPaths))
         Safe.push_back(CHI);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144927