[PATCH] D29092: PR31729: [GVNHoist] Don't hoist unsafe scalars at -Oz
Aditya Kumar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 23 16:49:22 PST 2017
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.
More information about the llvm-commits