[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#685265, @dberlin wrote:

> 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.
>  )


GVNHoist: 935 checks if  Call->mayThrow() which checks if the call will unwind or not IIUC.
Thanks for the feedback.


https://reviews.llvm.org/D29092