Page MenuHomePhabricator

[ValueTracking] Teach isSafeToSpeculativelyExecute() about the speculatable attribute
ClosedPublic

Authored by arsenm on Jul 15 2016, 9:45 AM.

Details

Diff Detail

Event Timeline

tstellarAMD retitled this revision from to [ValueTracking] Teach isSafeToSpeculativelyExecute() about the speculatable attribute.
tstellarAMD updated this object.
tstellarAMD added a subscriber: llvm-commits.

I am planning to add the speculatable attribute to all the intrinsics listed in this function in a separate patch.

Do we want to also change this function to give information about speculatable cost as proposed in this review: https://reviews.llvm.org/D20116

Another question I have is should the presence of speculatable by itself be enough for isSafeToSpeculativelyExecute() to return true. This would mean that the callers would be responsible for checking memory dependencies before speculating. The alternative would be to make isSafeToSpeculativelyExecute() require both the readnone and speculatable attributes.

arsenm edited edge metadata.Jul 15 2016, 9:59 AM

Doesn't the SpeculativeExecution already considered the TTI cost for this anyway?

lib/Analysis/ValueTracking.cpp
3120–3125

Is invoke allowed to be speculatable?

hfinkel accepted this revision.Jul 15 2016, 10:05 AM
hfinkel edited edge metadata.

Doesn't the SpeculativeExecution already considered the TTI cost for this anyway?

SimplifyCFG does, so we seem to be okay there.

This LGTM.

lib/Analysis/ValueTracking.cpp
3120–3125

No. The invoked function might throw. If it can't throw, then we'll convert it into a regular call during simplification.

This revision is now accepted and ready to land.Jul 15 2016, 10:05 AM
lib/Analysis/ValueTracking.cpp
3120–3125

So do I need to check for invoke here, or is it safe to assume that functions referenced by invoke will never have the speculatable attribute.?

hfinkel added inline comments.Jul 15 2016, 11:20 AM
lib/Analysis/ValueTracking.cpp
3120–3125

No, I don't think so.

arsenm added inline comments.Jul 15 2016, 11:28 AM
lib/Analysis/ValueTracking.cpp
3120–3125

A verifier check should probably be added in the other patch for this if it isn't already there

majnemer added inline comments.
lib/Analysis/ValueTracking.cpp
3121

I'd do auto * here.

3125–3128

Can a FIXME get added to mark these as speculatable in the tablegen file?

hfinkel added inline comments.Jul 15 2016, 11:31 AM
lib/Analysis/ValueTracking.cpp
3120–3125

Do you mean a verifier check that a function that might throw is marked as speculatable? I'm not sure that's a good idea.

tstellarAMD edited edge metadata.

Address some review comments.

tstellarAMD marked 2 inline comments as done.Jul 15 2016, 1:38 PM
arsenm added inline comments.Jul 15 2016, 1:40 PM
lib/Analysis/ValueTracking.cpp
3120–3125

I mean the opposite, that an invoke never has a speculatable

majnemer added inline comments.Jul 15 2016, 1:44 PM
lib/Analysis/ValueTracking.cpp
3120–3125

IMO, it seems like it would belong more in llvm's Lint analysis than a verifier check.

hfinkel added inline comments.Jul 15 2016, 2:40 PM
lib/Analysis/ValueTracking.cpp
3120–3125

I agree. We could easily end up with invoked functions being speculatable as an intermediate state (especially if we start inferring the attribute).

Also, it is not clearly a logical contradiction. The user might add the speculatable attribute on a function that might throw because the user does not care if the exception is thrown early. I'm not sure what we would do with that, however, because moving the catch blocks around to speculate the invoke seems unlikely worthwhile.

arsenm commandeered this revision.Apr 28 2017, 1:49 PM
arsenm edited reviewers, added: tstellarAMD; removed: arsenm.
arsenm closed this revision.Apr 28 2017, 2:26 PM

r301688