[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();
----------------
kpn wrote:
> nikic wrote:
> > 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.
> Having a special case here seems like a lower risk change. It can always be improved later. It would be nice to get strictfp improvements to IPSCCP unblocked.
Having the special case here is fine, but please limit it to `isa<LoadInst>(I)` and add a comment for why it is necessary (atomic loads from globals that are effectively constant, in a way that IPSCCP knows but wouldInstructionBeTriviallyDead does not). The current comment is unclear on why the additional checks are needed (and most of them are not needed).


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

https://reviews.llvm.org/D118387