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

[Nikita Popov via Phabricator via llvm-commits]

nikic 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:
> 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);
> ```
`readnone` means it's not allowed to read or write the global argument, so hoisting is indeed safe. As mentioned before, what you want here is `memory(argmem: readwrite)` to say that the callbr can access its arguments but will not clobber unrelated memory.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144927