[PATCH] D38856: [IPSCCP] Remove calls without side effects

[Davide Italiano via Phabricator via llvm-commits]

davide added inline comments.


================
Comment at: lib/Transforms/Scalar/SCCP.cpp:1929
         if (tryToReplaceWithConstant(Solver, Inst)) {
-          if (!isa<CallInst>(Inst) && !isa<TerminatorInst>(Inst))
+          if ((!isa<CallInst>(Inst) || !Inst->mayHaveSideEffects()) &&
+              !isa<TerminatorInst>(Inst))
----------------
beanz wrote:
> sanjoy wrote:
> > Can we just make this `!Inst->mayHaveSideEffects() && !isa<TerminatorInst>(Inst)`?  Otherwise we're assuming that non-calls that aren't terminators are not side effecting, which may be correct specifically for the instructions that `tryToReplaceWithConstant` returns true for, but isn't true in general.
> The problem with that is that we do have instructions with side effects that we want to fold away. For example we have `test/Transforms/SCCP/atomic-load-store.ll` which tests that we do erase atomic loads and stores.
This seems to be on the right track.
What I think is that this check is leaking too much information. I'd move it to a separate function, something called `isSafeToReplace()`.
I wonder if this should actually live in SCCP or in Instruction (or somewhere else), as the rules for folding away should be fairly general.


https://reviews.llvm.org/D38856