[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#685286, @efriedma wrote:

> There are two ways you can prove hoisting an instruction doesn't introduce undefined behavior.
>
> One is isSafeToSpeculativelyExecute.  This is obvious and uncontroversial.
>
> The other involves proving that if the hoisted instruction has undefined behavior, the program's behavior was undefined anyway.  For example, consider "if (x) { f(a/b); } else { g(a/b); }".  You can hoist a/b out of the if statement because if b is zero, you hit undefined behavior on either path.


Thanks for the feedback. GVNHoist does that. Please see the GVNHoist:298 hoistingFromAllPaths(). It will hoist instructions only when they are very busy. Only in the case of '-Os' the check was removed from scalars which introduced this bug. So I added
the check `isSafeToSpeculativelyExecute` to bail out quickly in this case by being conservative.

> Proving safety here is tricky; there are a lot of edge cases you need to deal with to get this right.  For example, you need to check individual instructions with isGuaranteedToTransferExecutionToSuccessor, and you need to handle potentially infinite loops.  (It's possible we could improve the IR to make this easier; see the discussion on https://reviews.llvm.org/D21041 .)
> 
> GVNHoist on trunk just blindly hoists anything, without trying to prove either condition.  This isn't causing real-world issues outside of OptForMinSize mode because, in an unrelated check, GVNHoist doesn't hoist anything past calls which write to memory.


https://reviews.llvm.org/D29092