[PATCH] D113105: [InstCombine] Extend stacksave/restore elimination

[Itay Bookstein via Phabricator via llvm-commits]

nextsilicon-itay-bookstein marked 2 inline comments as done.
nextsilicon-itay-bookstein added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1796-1797
+            return ClassifyResult::CallWithSideEffects;
+        } else {
+          return ClassifyResult::CallWithSideEffects;
+        }
----------------
nikic wrote:
> lebedev.ri wrote:
> > Maybe add a comment that all non-intrinsic calls are considered to be side-effects?
> Generally, an explanation how exactly "side effects" come into this would be nice. "Side effect" in LLVM is defined as memory write, unwind or divergence, none of which are really related to allocas (an alloca is not a side effect).
I added a comment about considering all non-intrinsic calls to be side-effects.
Regarding the allocas, I classify them separately above, so I'm not 100% sure if you meant that I should add something at this line or above?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1807
     if (IntrinsicInst *SS = dyn_cast<IntrinsicInst>(II->getArgOperand(0))) {
       if (SS->getIntrinsicID() == Intrinsic::stacksave) {
+        BasicBlock::iterator BI(SS);
----------------
lebedev.ri wrote:
> && SS.getParent() == II.getParent()
Great catch, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113105