Page MenuHomePhabricator

Teach optimizer that pthread_self does not trap. It can be speculatively executed.
AbandonedPublic

Authored by trentxintong on Nov 21 2016, 1:05 PM.

Details

Reviewers
hfinkel
Summary

Pass TargetLibraryInfo to isSafeToSpeculativelyExecute so that it can understand the behavior of known library functions.

With this patch, pthread_self is known to NOT trap, and therefore can be speculatively executed.

Diff Detail

Repository
rL LLVM

Event Timeline

trentxintong retitled this revision from to Teach optimizer that pthread_self does not trap. It can be speculatively executed..
trentxintong updated this object.
trentxintong added a reviewer: hfinkel.
trentxintong set the repository for this revision to rL LLVM.
trentxintong added a subscriber: llvm-commits.

Adding Hal because he has been answering questions on LICM.

joerg added a subscriber: joerg.Nov 21 2016, 3:00 PM

Why isn't attributing pthread_self as readonly or even readnone enough for this purpose?

trentxintong added a comment.EditedNov 21 2016, 3:11 PM

Because, if pthread_self (or a function) could trap and is not guaranteed to execute. we cant pull it to the preheader. And readonly/readnone + nonunwind function does not imply the function cant trap.

The conditions for LI code to be pulled to the preheader is.

  1. the operands of the instructions are invariant.
  2. the results produced by the instructions are loop invariant, e.g. invariant load or calls that will return the same value everytime.
  3. the instruction can be speculatively executed or is guaranteed to be executed.

Only know pthread_self is readnone+nounwind does not suffice. we need to teach the optimizer that it can not trap.

joerg added a comment.Nov 21 2016, 3:17 PM

Well, readnone functions are safe to speculatively execute. The question is if pthread_self() does qualify for it, since strictly it does read global memory. My problem with this change is that this seems to add a very stricted form of readnone in a non-systematic and adhoc fashion.

trentxintong added a comment.EditedNov 21 2016, 3:20 PM

readnone - On a function, this attribute indicates that the function computes its result (or decides to unwind an exception) based strictly on its arguments, without dereferencing any pointer arguments or otherwise accessing any mutable state ...

what if i have, this function is readnone and nounwind, but it can trap.

define int @unsafe_div(int a, int b) {

return a/b;

}

joerg added a comment.Nov 21 2016, 3:39 PM

Your example is UB.

I, too, hope readnone+nounwind functions can be speculatively executed, but it does not seem to be the case =). Thats why i think we need to use TLI to teach the optimizer about functions which can.

hfinkel edited edge metadata.Nov 21 2016, 3:43 PM

I, too, hope readnone+nounwind functions can be speculatively executed, but it does not seem to be the case =). Thats why i think we need to use TLI to teach the optimizer about functions which can.

Your example is UB.

You're both right. We cannot speculate functions that might have UB. Thus, we cannot speculate general external functions (even if they're readnone, etc.). See also: D20116 where we're discussing adding a separate attribute for this.

I, too, hope readnone+nounwind functions can be speculatively executed, but it does not seem to be the case =). Thats why i think we need to use TLI to teach the optimizer about functions which can.

Your example is UB.

You're both right. We cannot speculate functions that might have UB. Thus, we cannot speculate general external functions (even if they're readnone, etc.). See also: D20116 where we're discussing adding a separate attribute for this.

@hfinkel I see, do you think it would be a good to add "speculatable" attribute to pthread_self in FuncAttr once the patch for the attribute lands so that it can be hoisted ? or we can use TLI in isSafeToSpeculativelyExecute function.

david2050 removed a subscriber: david2050.
david2050 added a subscriber: david2050.

I, too, hope readnone+nounwind functions can be speculatively executed, but it does not seem to be the case =). Thats why i think we need to use TLI to teach the optimizer about functions which can.

Your example is UB.

You're both right. We cannot speculate functions that might have UB. Thus, we cannot speculate general external functions (even if they're readnone, etc.). See also: D20116 where we're discussing adding a separate attribute for this.

@hfinkel I see, do you think it would be a good to add "speculatable" attribute to pthread_self in FuncAttr once the patch for the attribute lands so that it can be hoisted ? or we can use TLI in isSafeToSpeculativelyExecute function.

I'd prefer just to use the attribute.

I, too, hope readnone+nounwind functions can be speculatively executed, but it does not seem to be the case =). Thats why i think we need to use TLI to teach the optimizer about functions which can.

Your example is UB.

You're both right. We cannot speculate functions that might have UB. Thus, we cannot speculate general external functions (even if they're readnone, etc.). See also: D20116 where we're discussing adding a separate attribute for this.

@hfinkel I see, do you think it would be a good to add "speculatable" attribute to pthread_self in FuncAttr once the patch for the attribute lands so that it can be hoisted ? or we can use TLI in isSafeToSpeculativelyExecute function.

I'd prefer just to use the attribute.

@hfinkel Ok, SGTM. Thanks for the pointer.

trentxintong abandoned this revision.Feb 26 2017, 11:23 PM

This patch needs to be completely rewritten (different approach) after the speculatable attribute lands.