[PATCH] D118387: [IPSCCP] Switch away from Instruction::isSafeToRemove()

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:98
+
+  return (!isa<CallInst>(I) || !I->mayHaveSideEffects()) &&
+         !I->isTerminator() && !I->isEHPad();
----------------
fhahn wrote:
> fhahn wrote:
> > Why is this extra code needed?
> > 
> > It looks like `wouldInstructionBeTriviallyDead` doesn't consider atomic loads from globals as trivially dead, which is causing an issue with IPSCCP as it requires such loads to be removed.
> > 
> > According to https://llvm.org/docs/Atomics.html#atomics-and-ir-optimization, atomic loads from globals can be constant folded, so I *think* `wouldInstructionBeTriviallyDead` should consider them as trivially dead.
> > @fhahn, are you referring to the body of Instruction::isSafeToRemove() that has been lifted up into this function here? We were using I->isSafeToRemove() before so this movement into this new wrapper keeps the behavior pretty much the same. IPSCCP's handling of atomic loads shouldn't be affected since a rejection by wouldInstructionBeTriviallyDead() falls through into the same code as before. Can we build on this to improve wouldInstructionBeTriviallyDead() later?
> 
> Yes I meant the inlined code here. Without comment, it is very hard for the reader to understand why it is there and why `wouldInstructionBeTriviallyDead` is not sufficient. I think it is fine to have some extra checks here before moving them to `wouldInstructionBeTriviallyDead`, but it should be as minimal as possible, with an explantation and a TODO.
I've put up https://reviews.llvm.org/D124241 to add the additional check to wouldInstructionBeTriviallyDead(). Unfortunately, this doesn't fully address the problem, because IPSCCP can also remove loads from non-constant globals if it determines that they are actually constant. We would either have to retain a special case here, or we'd have to mark the globals as constant before attempting to remove uses.


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

https://reviews.llvm.org/D118387