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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:98
+
+  return (!isa<CallInst>(I) || !I->mayHaveSideEffects()) &&
+         !I->isTerminator() && !I->isEHPad();
----------------
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.


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

https://reviews.llvm.org/D118387