[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]
----------------
nikic wrote:
> 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.
What's the difference between `memory(argmem: readwrite)` vs no attribute?  I guess https://llvm.org/docs/LangRef.html#function-attributes says that "memory(readwrite)" is the implicit default?

Shouldn't the front end be adding such attributes?
https://godbolt.org/z/zxM36Esd9

I'm happy to add whatever attributes to these test cases, my concern is that is feels like we're adding test cases for IR that frontend may never produce. Is that worthwhile to test? I kind of doubt it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144927