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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

In https://reviews.llvm.org/D29092#685017, @majnemer wrote:

> In https://reviews.llvm.org/D29092#682961, @hans wrote:
>
> > David, do you have any further concerns? Otherwise I'd like to see this committed so we can resolve the PR for 4.0.
>
>
> I don't understand why we cannot assert isSafeToSpeculativelyExecute if we are not in minsize. If we are given a call instruction, why do we think it is safe to hoist it?






================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:510
     // where they are partially needed.
-    if (OptForMinSize)
+    if (OptForMinSize && isSafeToSpeculativelyExecute(I))
       return true;
----------------
MatzeB wrote:
> hiraditya wrote:
> > majnemer wrote:
> > > Shouldn't we unconditionally do `isSafeToSpeculativelyExecute`?
> > GVNHoist already has facilities to check the dependencies, safety etc. which is more efficient than calling isSafeToSpeculativelyExecute all the time for each instruction.
> Sounds like at least an `assert(isSafeToSpeculativelyExecute());` is a good idea.
The function name is `safeToHoistScalar`, I don't understand why `OptForMinSize` is checked at all here. There shouldn't be *any* profitability involved in this function, according to its name and its description.


https://reviews.llvm.org/D29092