[PATCH] D29092: PR31729: [GVNHoist] Don't hoist unsafe scalars at -Oz

[Aditya Kumar via Phabricator via llvm-commits]

hiraditya added a comment.

In https://reviews.llvm.org/D29092#685417, @dberlin wrote:

> In https://reviews.llvm.org/D29092#685416, @hiraditya wrote:
>
> > In https://reviews.llvm.org/D29092#685323, @efriedma wrote:
> >
> > > > Thanks for the feedback. GVNHoist does that. Please see the GVNHoist:298 hoistingFromAllPaths(). It will hoist instructions only when they are very busy.
> > >
> > > Oh, okay.  That's... the right sort of thing, but I think it's missing some checks.  The most obvious hole is that you need to call isGuaranteedToTransferExecutionToSuccessor on each instruction in the block, not just the terminator.  It also looks like it doesn't handle infinite loops correctly.
> >
> >
> > It checks for Branches with isGuaranteedToTransferExecutionToSuccessor, other than that there are memory instructions and calls in the definition of isGuaranteedToTransferExecutionToSuccessor, which are handled separately (with some help from MemorySSA) here. So there is no need to call this function for each instruction.
>
>
> Can you please clearly map for us the things that function checks for to where you are doing it here?
>  Literally, i mean?


isGuaranteedToTransferExecutionToSuccessor checks for three classes of instructions:

1. TerminatorInst
2. Memory operations
3. Calls

For TerminatorInst, GVNHoist is using isGuaranteedToTransferExecutionToSuccessor
Memory operations are handled in safeToHoistLdSt which uses MemorySSA which returns/should return the MemoryAccess which might alter a memory operation (GVNHoist.cpp:478)
Calls are also handled separately in GVNHoist with partial help from MemorySSA GVNHoist.cpp:385, 492, 494

> Honestly, i think a large amount of the commentary on this review would have been avoided with clear, concise, and centralized code that mapped existing and well known idioms for doing these things in llvm into the way you achieve the same result.

The reason to handle them separately is because:

1. We would need to call isGuaranteedToTransferExecutionToSuccessor for all instructions in path which won't be efficient
2. MemorySSA does most parts of isGuaranteedToTransferExecutionToSuccessor in more efficient way by iterating over instructions which might affect memory.


https://reviews.llvm.org/D29092