[IPSCCP] Remove calls without side effects

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



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.

Diff Detail

beanz created this revision.Oct 12 2017, 10:47 AM
sanjoy added inline comments.Oct 12 2017, 1:00 PM
1929 ↗(On Diff #118809)

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.Oct 12 2017, 5:39 PM
1929 ↗(On Diff #118809)

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.Oct 12 2017, 7:51 PM
1929 ↗(On Diff #118809)

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.
1929 ↗(On Diff #118809)

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.Oct 12 2017, 10:12 PM
1929 ↗(On Diff #118809)

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

beanz updated this revision to Diff 126414.Dec 11 2017, 11:38 AM

Abstracting check for whether or not it is safe to remove an instruction as per the feedback from davide.

None of the other feedback seems to have consensus. Anything else I should change here or can this land?


davide accepted this revision.Dec 11 2017, 1:32 PM

I don't think this function is general enough to live in Instruction.h, I'd keep it private to SCCP for now.
I think we need to reach consensus on how this will look like in the end, but this is probably fine for now.
I'd give @majnemer / @dberlin another change of commenting, but I guess this is fine to get in.

This revision is now accepted and ready to land.Dec 11 2017, 1:32 PM
Closed by commit rL322125: [IPSCCP] Remove calls without side effects (authored by cbieneman, committed by ). · Explain WhyJan 9 2018, 1:59 PM
This revision was automatically updated to reflect the committed changes.