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

[Aditya Kumar via Phabricator via llvm-commits]

hiraditya added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:510
     // where they are partially needed.
-    if (OptForMinSize)
+    if (OptForMinSize && isSafeToSpeculativelyExecute(I))
       return true;
----------------
mehdi_amini wrote:
> 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.
I agree, I can put this check outside if the function safeToHoistScalar, it is seems more reasonable.


https://reviews.llvm.org/D29092