[IPSCCP] Remove calls without side effects
Needs ReviewPublic

Authored by beanz on Thu, Oct 12, 10:47 AM.

Details

Summary

When performing constant propagation for call instructions we have historically replaced all uses of the return from a call, but not removed the call itself. This is required for correctness if the calls have side effects, however the compiler should be able to safely remove calls that don't have side effects.

This allows the compiler to completely fold away calls to functions that have no side effects if the inputs are constant and the output can be determined at compile time.

beanz created this revision.Thu, Oct 12, 10:47 AM
sanjoy added inline comments.Thu, Oct 12, 1:00 PM
lib/Transforms/Scalar/SCCP.cpp
1929

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.

beanz added inline comments.Thu, Oct 12, 5:39 PM
lib/Transforms/Scalar/SCCP.cpp
1929

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.

davide added inline comments.Thu, Oct 12, 7:51 PM
lib/Transforms/Scalar/SCCP.cpp
1929

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.

majnemer added inline comments.
lib/Transforms/Scalar/SCCP.cpp
1929

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.

dberlin added inline comments.Thu, Oct 12, 10:12 PM
lib/Transforms/Scalar/SCCP.cpp
1929

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 || ....