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

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

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?

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.


https://reviews.llvm.org/D29092