[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#685256, @hiraditya wrote:

> In https://reviews.llvm.org/D29092#685046, @mehdi_amini wrote:
>
> > I'd add also that `isSafeToSpeculativelyExecute` may be conservative right now, but it should improve with https://reviews.llvm.org/D20116
> >  So it is even unclear to me which cases GVN hoist would be able to catch that are not in the realm of  `isSafeToSpeculativelyExecute`.
>
>
> Currently `isSafeToSpeculativelyExecute` returns false for Calls, stores. GVN Hoist will hoist them if it is legal to do so.


The concern is precisely that it does not do so correctly.
In fact, it definitely does not.
As a trivial example: Nowhere, does GVNHoist check for whether calls unwind or not.
It checks for "EH on a path", but it doesn't check this, for example.

Now, actually, i do not believe isSafeToSpeculativelyExecute is the right answer here longer term, because it is easy to do better than it.
But you at least must be as correct as it.

(Though i quibble with this part:
 // The called function could have undefined behavior or
 // side-effects, even if marked readnone nounwind.

I'm not sure what we think those side effects are, but we should be modeling them or ignoring them.
)


https://reviews.llvm.org/D29092