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

[Daniel Berlin via Phabricator via llvm-commits]

dberlin 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))
----------------
majnemer wrote:
> 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.
That would not return true for anything with side effects. So it will not handle the atomic loads and stores cases.  mayHaveSideEffects will return true for anything that might write to memory.  mayWriteToMemory will return true for any store, and any ordered load.

In fact, even getModRefInfo wouldn't help you with that here, it would do the same, and can't be overridden by anything more powerful right now (we only have an AA interface for callsites, the regular instructions are all single implementation in AliasAnalysis.cpp)

So TL; DR if you wanted to use isInstructionTriviallyDead, it would be something like isInstructionTriviallyDead || is a load || is a store || atomicrmw || cmpxchng || ....




https://reviews.llvm.org/D38856