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

[David Majnemer via Phabricator via llvm-commits]

majnemer 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))
----------------
davide wrote:
> 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.
Could we just call `isInstructionTriviallyDead`? It already knows about terminators and all the other stuff. There is also `wouldInstructionBeTriviallyDead` if you want to query before the replacement.


https://reviews.llvm.org/D38856