Add pthread_self function prototype and make it speculatable.

[trentxintong]

Summary

This allows pthread_self to be pulled out of a loop by LICM.

[trentxintong]

Add tests

[davide]

lib/Analysis/TargetLibraryInfo.cpp

1182–1184

I'm not entirely sure about this bit.
IIRC POSIX specifies thread ids to be opaque, see:

Thread identifiers should be considered opaque: any attempt to use a
thread ID other than in pthreads calls is nonportable and can lead to
unspecified results.

so this assumption of it being an integer type might not hold.

[trentxintong]

lib/Analysis/TargetLibraryInfo.cpp

1182–1184

Yes, I am a bit ambivalent on this as well when i wrote the patch.
We find library functions by matching its name and its specific function signature. I guess I could leave out the check for the argument here as its opaque and may vary from platform to platform.
There is a test case in unittest I need to fix if we do this.

[trentxintong]

Do not check for return value for pthread_self.

[trentxintong]

Update 1 test.

[arsenm]

test/Transforms/LICM/pthread.ll

7

Doesn't actually check for speculatable on declaration

[trentxintong]

Update 1 test.

[trentxintong]

Make the LIT checks tighter.

[trentxintong]

Friendly Ping.

[trentxintong]

Ping.

[davide]

Do you have some C code where this triggers? Can you provide an example?

[trentxintong]

I have an internal synthetic benchmark which is not very interesting. But I imagine its not difficult to come up with a case which a function calls pthread_self on entry and the function is called within a loop. After inlining, the pthread_self is called inside the loop.

BENCHMARK(pthread_self, n) {

for (int i = 0; i < n; i++) {
  auto id = pthread_self();
  folly::doNotOptimizeAway(id);
}

}

[davide]

Looks okay, my POSIX was rusty so I looked at it again and this seems OK to speculate as it has no side-effects.
The thread id is guaranteed to be unique across all *running* threads, but can be reused when a threads joins and another one is created. I don't think this matters for this optimization.
I looked very closely at the opengroup spec and I can't find a paragraph stating that the return of pthread_self() is guaranteed to be unique for the thread lifetime.
I expect any sane implementation to do that, and it's probably implicit.

[trentxintong]

BTW, GCC moves pthread_self out of the loop on Linux. so I would think this is legal on linux. Unless GCC handles pthread_self differently on different platforms, I think this would be fine.

Also, if you read this carefully http://h20564.www2.hpe.com/hpsc/doc/public/display?docId=emr_na-c02267692&lang=en-us&cc=us

Its saying the "thread ID returned is the same ID that is returned in the thread parameter to the creating thread at thread creation time", basically implying the thread ID returned by pthread_self stays the same

[davide]

In D32782#760260, @trentxintong wrote:

BTW, GCC moves pthread_self out of the loop on Linux. so I would think this is legal on linux. Unless GCC handles pthread_self differently on different platforms, I think this would be fine.

Also, if you read this carefully http://h20564.www2.hpe.com/hpsc/doc/public/display?docId=emr_na-c02267692&lang=en-us&cc=us

Its saying the "thread ID returned is the same ID that is returned in the thread parameter to the creating thread at thread creation time", basically implying the thread ID returned by pthread_self stays the same

That's just the HP-UX documentation, not the standard.
http://pubs.opengroup.org/onlinepubs/9699919799/

Anyway, after all this elucubration, LGTM.

[davide]

OK, I think I found out the cause. I guess this patch was wrong, my bad.
GCC doesn't do anything special with pthread_self per-se.
What GCC does is speculating the glibc implementation of pthread_self is declared with __attribute__(const).
The semantic of the attribute is that of "The const attribute is specified as asserting that the function does not examine any data except the arguments. " [1]
If the function has no arguments, it has to return the same value every time.

Therefore, it speculates.
I think that other libc implementation are free to not declare pthread_self with that attribute. In fact, from what I can see, the FreeBSD version doesn't use that argument.
In other words, I don't think we're allowed to do anything with pthread_self() in general as POSIX specifies weaks guarantees.

[1] https://sourceware.org/ml/libc-alpha/2016-04/msg00303.html

[davide]

Note: whether it's a good idea to declare pthread_self() with attribute const is a different story, but I'll leave the answer to the glibc developers :)

[davide]

+ @chandlerc and @joerg as they have opinion on the topic.

[hfinkel]

In D32782#760304, @davide wrote:

OK, I think I found out the cause. I guess this patch was wrong, my bad.
GCC doesn't do anything special with pthread_self per-se.
What GCC does is speculating the glibc implementation of pthread_self is declared with __attribute__(const).
The semantic of the attribute is that of "The const attribute is specified as asserting that the function does not examine any data except the arguments. " [1]
If the function has no arguments, it has to return the same value every time.

Therefore, it speculates.
I think that other libc implementation are free to not declare pthread_self with that attribute. In fact, from what I can see, the FreeBSD version doesn't use that argument.
In other words, I don't think we're allowed to do anything with pthread_self() in general as POSIX specifies weaks guarantees.

So there a general implication we can implement: __attribute__(const). + zero arguments == speculatable?

Also, I fail to see how it would not be safe to tread pthread_self as speculatable? Same for getpid.

The standard says that the call always succeeds and always returns the thread id. The thread ids are opaque, and I can imagine there being multiple "self" values (pthread_equal would just return true for all of them), thus making pthread_self non-const. However, I can think of no reason why an implementation would do this, don't know of any that do, the behavior would only be observable via some non-standard interface, and I'm happy to cross that bridge if we come to it.

All of that having been said, however, I think there is something we need to clarify about the semantics. If we allow the transformation:

static pthread_t tid;

int main() {
  tid = pthread_self();
  ...
}

void foo() {
  auto x =tid;
  bar(x);
  ...
}

to:

int main() {
  ...
}

void foo() {
  auto x =pthread_self();
  bar(x);
  ...
}

this might obviously cause problems (if foo() is called from a different thread than main). This transformation might even be reasonable for 'speculatable' functions that are cheap (as pthread_self should be).

So the semantics we need here are a little less than speculatable, or const, put somehow restricted in scope to function-local transformations.

[1] https://sourceware.org/ml/libc-alpha/2016-04/msg00303.html

[davide]

In D32782#760309, @hfinkel wrote:

In D32782#760304, @davide wrote:

OK, I think I found out the cause. I guess this patch was wrong, my bad.
GCC doesn't do anything special with pthread_self per-se.
What GCC does is speculating the glibc implementation of pthread_self is declared with __attribute__(const).
The semantic of the attribute is that of "The const attribute is specified as asserting that the function does not examine any data except the arguments. " [1]
If the function has no arguments, it has to return the same value every time.

Therefore, it speculates.
I think that other libc implementation are free to not declare pthread_self with that attribute. In fact, from what I can see, the FreeBSD version doesn't use that argument.
In other words, I don't think we're allowed to do anything with pthread_self() in general as POSIX specifies weaks guarantees.

So there a general implication we can implement: __attribute__(const). + zero arguments == speculatable?

I'm not sure if that's the exact recipe, but yes, it seems to be on the right path.

Also, I fail to see how it would not be safe to tread pthread_self as speculatable? Same for getpid.

The standard says that the call always succeeds and always returns the thread id. The thread ids are opaque, and I can imagine there being multiple "self" values (pthread_equal would just return true for all of them), thus making pthread_self non-const. However, I can think of no reason why an implementation would do this, don't know of any that do, the behavior would only be observable via some non-standard interface, and I'm happy to cross that bridge if we come to it.

I don't understand the bit about getpid(). In that case forking actually could change the value and you might end up in trouble if you rely on that to write temporary directories (as it's generally done).

All of that having been said, however, I think there is something we need to clarify about the semantics. If we allow the transformation:

static pthread_t tid;

int main() {
  tid = pthread_self();
  ...
}

void foo() {
  auto x =tid;
  bar(x);
  ...
}

to:

int main() {
  ...
}

void foo() {
  auto x =pthread_self();
  bar(x);
  ...
}

this might obviously cause problems (if foo() is called from a different thread than main). This transformation might even be reasonable for 'speculatable' functions that are cheap (as pthread_self should be).

So the semantics we need here are a little less than speculatable, or const, put somehow restricted in scope to function-local transformations.

[1] https://sourceware.org/ml/libc-alpha/2016-04/msg00303.html

[hfinkel]

In D32782#760311, @davide wrote:

In D32782#760309, @hfinkel wrote:

In D32782#760304, @davide wrote:

OK, I think I found out the cause. I guess this patch was wrong, my bad.
GCC doesn't do anything special with pthread_self per-se.
What GCC does is speculating the glibc implementation of pthread_self is declared with __attribute__(const).
The semantic of the attribute is that of "The const attribute is specified as asserting that the function does not examine any data except the arguments. " [1]
If the function has no arguments, it has to return the same value every time.

Therefore, it speculates.
I think that other libc implementation are free to not declare pthread_self with that attribute. In fact, from what I can see, the FreeBSD version doesn't use that argument.
In other words, I don't think we're allowed to do anything with pthread_self() in general as POSIX specifies weaks guarantees.

So there a general implication we can implement: __attribute__(const). + zero arguments == speculatable?

I'm not sure if that's the exact recipe, but yes, it seems to be on the right path.

Also, I fail to see how it would not be safe to tread pthread_self as speculatable? Same for getpid.

The standard says that the call always succeeds and always returns the thread id. The thread ids are opaque, and I can imagine there being multiple "self" values (pthread_equal would just return true for all of them), thus making pthread_self non-const. However, I can think of no reason why an implementation would do this, don't know of any that do, the behavior would only be observable via some non-standard interface, and I'm happy to cross that bridge if we come to it.

I don't understand the bit about getpid(). In that case forking actually could change the value and you might end up in trouble if you rely on that to write temporary directories (as it's generally done).

Oh. You're right. Also, that seems to also rule out this as well. fork() could also change the value of pthread_self() I'd imagine.

[davide]

In D32782#760312, @hfinkel wrote:

In D32782#760311, @davide wrote:

In D32782#760309, @hfinkel wrote:

In D32782#760304, @davide wrote:

OK, I think I found out the cause. I guess this patch was wrong, my bad.
GCC doesn't do anything special with pthread_self per-se.
What GCC does is speculating the glibc implementation of pthread_self is declared with __attribute__(const).
The semantic of the attribute is that of "The const attribute is specified as asserting that the function does not examine any data except the arguments. " [1]
If the function has no arguments, it has to return the same value every time.

Therefore, it speculates.
I think that other libc implementation are free to not declare pthread_self with that attribute. In fact, from what I can see, the FreeBSD version doesn't use that argument.
In other words, I don't think we're allowed to do anything with pthread_self() in general as POSIX specifies weaks guarantees.

So there a general implication we can implement: __attribute__(const). + zero arguments == speculatable?

I'm not sure if that's the exact recipe, but yes, it seems to be on the right path.

Also, I fail to see how it would not be safe to tread pthread_self as speculatable? Same for getpid.

The standard says that the call always succeeds and always returns the thread id. The thread ids are opaque, and I can imagine there being multiple "self" values (pthread_equal would just return true for all of them), thus making pthread_self non-const. However, I can think of no reason why an implementation would do this, don't know of any that do, the behavior would only be observable via some non-standard interface, and I'm happy to cross that bridge if we come to it.

I don't understand the bit about getpid(). In that case forking actually could change the value and you might end up in trouble if you rely on that to write temporary directories (as it's generally done).

Oh. You're right. Also, that seems to also rule out this as well. fork() could also change the value of pthread_self() I'd imagine.

It's slightly different, at least in my opinion.
getpid() returns a pid_t.

pid_t is defined to be an integer type, although POSIX doesn't put any restrictions on the size.
http://pubs.opengroup.org/onlinepubs/009696699/basedefs/sys/types.h.html

glibc (and FreeBSD libc) decide to make it an int. People know it's an integer and use as such.

On the other hand, pthread_t is considered to be an opaque type. It can be an integer/a struct/you name it.
In fact, this completely opaque implementation detail wildly varies across implementations (for LinuxThreads, it's an integer, for NPTL, a pointer).
http://man7.org/linux/man-pages/man3/pthread_self.3.html
Linux also documents that using pthread_t in anything that's not pthread calls results in an unspecified behaviour, while pid_t can be used freely.

So, it's not quite the same, but it's still debatable whether the transformation should be performed or not.

[hfinkel]

In D32782#760320, @davide wrote:

In D32782#760312, @hfinkel wrote:

In D32782#760311, @davide wrote:

In D32782#760309, @hfinkel wrote:

In D32782#760304, @davide wrote:

OK, I think I found out the cause. I guess this patch was wrong, my bad.
GCC doesn't do anything special with pthread_self per-se.
What GCC does is speculating the glibc implementation of pthread_self is declared with __attribute__(const).
The semantic of the attribute is that of "The const attribute is specified as asserting that the function does not examine any data except the arguments. " [1]
If the function has no arguments, it has to return the same value every time.

Therefore, it speculates.
I think that other libc implementation are free to not declare pthread_self with that attribute. In fact, from what I can see, the FreeBSD version doesn't use that argument.
In other words, I don't think we're allowed to do anything with pthread_self() in general as POSIX specifies weaks guarantees.

So there a general implication we can implement: __attribute__(const). + zero arguments == speculatable?

I'm not sure if that's the exact recipe, but yes, it seems to be on the right path.

Also, I fail to see how it would not be safe to tread pthread_self as speculatable? Same for getpid.

The standard says that the call always succeeds and always returns the thread id. The thread ids are opaque, and I can imagine there being multiple "self" values (pthread_equal would just return true for all of them), thus making pthread_self non-const. However, I can think of no reason why an implementation would do this, don't know of any that do, the behavior would only be observable via some non-standard interface, and I'm happy to cross that bridge if we come to it.

I don't understand the bit about getpid(). In that case forking actually could change the value and you might end up in trouble if you rely on that to write temporary directories (as it's generally done).

Oh. You're right. Also, that seems to also rule out this as well. fork() could also change the value of pthread_self() I'd imagine.

It's slightly different, at least in my opinion.
getpid() returns a pid_t.

pid_t is defined to be an integer type, although POSIX doesn't put any restrictions on the size.
http://pubs.opengroup.org/onlinepubs/009696699/basedefs/sys/types.h.html

glibc (and FreeBSD libc) decide to make it an int. People know it's an integer and use as such.

On the other hand, pthread_t is considered to be an opaque type. It can be an integer/a struct/you name it.
In fact, this completely opaque implementation detail wildly varies across implementations (for LinuxThreads, it's an integer, for NPTL, a pointer).
http://man7.org/linux/man-pages/man3/pthread_self.3.html
Linux also documents that using pthread_t in anything that's not pthread calls results in an unspecified behaviour, while pid_t can be used freely.

So, it's not quite the same, but it's still debatable whether the transformation should be performed or not.

I understand all of this, but the underlying issue is still the same: you can't, in general, move the call past fork(). I see no reason that pthread_equal on the pre-fork and post-fork values would always return true (although this seems to be the case on Linux, even if you fork from a child thread).

[joerg]

Strictly speaking, the glibc attribute violates the specification of const. I.e. we could turn a call for pthread_self() into a once() initialised static variable and that transform would be valid under the const rules. That clearly doesn't reflect the intention...

That said, I do believe it is fully reasonable to make pthread_self() speculatible by default as long as it can be turned off and is properly documented.

include/llvm/Analysis/TargetLibraryInfo.def

 TLI_DEFINE_ENUM_INTERNAL(powl)
 TLI_DEFINE_STRING_INTERNAL("powl")
 /// ssize_t pread(int fildes, void *buf, size_t nbyte, off_t offset);
 TLI_DEFINE_ENUM_INTERNAL(pread)
 TLI_DEFINE_STRING_INTERNAL("pread")
 /// int printf(const char *format, ...);
 TLI_DEFINE_ENUM_INTERNAL(printf)
 TLI_DEFINE_STRING_INTERNAL("printf")
+/// pthread_t pthread_self(void);
+TLI_DEFINE_ENUM_INTERNAL(pthread_self)
+TLI_DEFINE_STRING_INTERNAL("pthread_self")
 /// int putc(int c, FILE *stream);
 TLI_DEFINE_ENUM_INTERNAL(putc)
 TLI_DEFINE_STRING_INTERNAL("putc")
 /// int putchar(int c);
 TLI_DEFINE_ENUM_INTERNAL(putchar)
 TLI_DEFINE_STRING_INTERNAL("putchar")
 /// int puts(const char *s);
 TLI_DEFINE_ENUM_INTERNAL(puts)

lib/Analysis/TargetLibraryInfo.cpp

if (T.isOSWindows() && !T.isOSCygMing()) {
     TLI.setUnavailable(LibFunc_utimes);
     TLI.setUnavailable(LibFunc_write);
 
     // Win32 does *not* provide provide these functions, but they are
     // specified by C99:
     TLI.setUnavailable(LibFunc_atoll);
     TLI.setUnavailable(LibFunc_frexpf);
     TLI.setUnavailable(LibFunc_llabs);
+
+    // Win32 does *not* provide pthread_self.
+    TLI.setUnavailable(LibFunc_pthread_self);
   }
 
   switch (T.getOS()) {
   case Triple::MacOSX:
     // exp10 and exp10f are not available on OS X until 10.9 and iOS until 7.0
     // and their names are __exp10 and __exp10f. exp10l is not available on
     // OS X or iOS.
     TLI.setUnavailable(LibFunc_exp10l);
return (NumParams == 2 && FTy.getReturnType() == FTy.getParamType(1) &&
             FTy.getParamType(0) == PCharTy &&
             FTy.getParamType(1) == SizeTTy);
 
   case LibFunc_posix_memalign:
     return (NumParams == 3 && FTy.getReturnType()->isIntegerTy(32) &&
             FTy.getParamType(0)->isPointerTy() &&
             FTy.getParamType(1) == SizeTTy && FTy.getParamType(2) == SizeTTy);
 
+  // We do not attempt to match the return value here. i.e. thread identifiers
+  // should be considered opaque, for example, representation using either an
+  // arithmetic type or a structure is permitted. 
+  case LibFunc_pthread_self:
+    return NumParams == 0;
+
   case LibFunc::NumLibFuncs:
     break;
   }
 
   llvm_unreachable("Invalid libfunc");
 }
 
 bool TargetLibraryInfoImpl::getLibFunc(const Function &FDecl,

lib/Transforms/Utils/BuildLibCalls.cpp

 STATISTIC(NumReadNone, "Number of functions inferred as readnone");
 STATISTIC(NumReadOnly, "Number of functions inferred as readonly");
 STATISTIC(NumArgMemOnly, "Number of functions inferred as argmemonly");
 STATISTIC(NumNoUnwind, "Number of functions inferred as nounwind");
 STATISTIC(NumNoCapture, "Number of arguments inferred as nocapture");
 STATISTIC(NumReadOnlyArg, "Number of arguments inferred as readonly");
 STATISTIC(NumNoAlias, "Number of function returns inferred as noalias");
 STATISTIC(NumNonNull, "Number of function returns inferred as nonnull returns");
+STATISTIC(NumSpeculatable, "Number of functions inferred as speculatable");
 
 static bool setDoesNotAccessMemory(Function &F) {
   if (F.doesNotAccessMemory())
     return false;
   F.setDoesNotAccessMemory();
   ++NumReadNone;
   return true;
 }

 static bool setDoesNotThrow(Function &F) {
   if (F.doesNotThrow())
     return false;
   F.setDoesNotThrow();
   ++NumNoUnwind;
   return true;
 }
 
+static bool setSpeculatable(Function &F) {
+  if (F.isSpeculatable())
+    return false;
+  F.setSpeculatable();
+  ++NumSpeculatable;
+  return true;
+}
+
 static bool setDoesNotCapture(Function &F, unsigned n) {
   if (F.doesNotCapture(n))
     return false;
   F.setDoesNotCapture(n);
   ++NumNoCapture;
   return true;
 }
 
case LibFunc_vscanf:
     return Changed;
   case LibFunc_vsscanf:
     Changed |= setDoesNotThrow(F);
     Changed |= setDoesNotCapture(F, 1);
     Changed |= setDoesNotCapture(F, 2);
     Changed |= setOnlyReadsMemory(F, 1);
     Changed |= setOnlyReadsMemory(F, 2);
     return Changed;
+  case LibFunc_pthread_self:
+    Changed |= setSpeculatable(F);
+    return Changed;
   case LibFunc_vfscanf:
     Changed |= setDoesNotThrow(F);
     Changed |= setDoesNotCapture(F, 1);
     Changed |= setDoesNotCapture(F, 2);
     Changed |= setOnlyReadsMemory(F, 2);
     return Changed;
   case LibFunc_valloc:
     Changed |= setDoesNotThrow(F);

test/Transforms/LICM/pthread.ll

+; RUN: opt < %s -S  -inferattrs -licm | FileCheck %s
+
+; CHECK-LABEL: define void @pthread_self_safe(
+; CHECK-NEXT: call i64 @pthread_self() 
+define void @pthread_self_safe(i32) {
+  br label %2
+
+; <label>:2:                                      ; preds = %7, %1
+  %idx = phi i32 [ 0, %1 ], [ %8, %7 ]
+  %3 = icmp slt i32 %idx, %0
+  br i1 %3, label %4, label %9
+
+; <label>:4:                                      ; preds = %2
+  call void @external_func_that_could_do_anything()
+  %5 = call i64 @pthread_self() #1
+  %6 = trunc i64 %5 to i32
+  call void @use_pthread_self(i32 %6)
+  br label %7
+
+; <label>:7:                                      ; preds = %4
+  %8 = add nsw i32 %idx, 1
+  br label %2
+
+; <label>:9:                                      ; preds = %2
+  ret void
+}
+
+; CHECK: declare i64 @pthread_self() #0
+; CHECK: attributes #0 = { nounwind readnone speculatable }
+; Function Attrs: nounwind readnone
+declare i64 @pthread_self() #1
+
+declare void @external_func_that_could_do_anything()
+
+declare void @use_pthread_self(i32)
+
+attributes #1 = { nounwind readnone }
+

unittests/Analysis/TargetLibraryInfoTest.cpp

 
 } // end anonymous namespace
 
 // Check that we don't accept egregiously incorrect prototypes.
 TEST_F(TargetLibraryInfoTest, InvalidProto) {
   parseAssembly("%foo = type { %foo }\n");
 
   auto *StructTy = M->getTypeByName("foo");
-  auto *InvalidFTy = FunctionType::get(StructTy, /*isVarArg=*/false);
-
   for (unsigned FI = 0; FI != LibFunc::NumLibFuncs; ++FI) {
     LibFunc LF = (LibFunc)FI;
+    // Using the library function name to create a function that takes
+    // 1 parameter and returns the same type. There should be no library
+    // function that matches this egregiously incorrect prototypes.
     auto *F = cast<Function>(
-        M->getOrInsertFunction(TLI.getName(LF), InvalidFTy));
+        M->getOrInsertFunction(TLI.getName(LF), StructTy, StructTy));
     EXPECT_FALSE(isLibFunc(F, LF));
   }
 }
 
 // Check that we do accept know-correct prototypes.
 TEST_F(TargetLibraryInfoTest, ValidProto) {
   parseAssembly(
     // These functions use a 64-bit size_t; use the appropriate datalayout.
parseAssembly(
     "declare x86_fp80 @nearbyintl(x86_fp80)\n"
     "declare i32 @pclose(%struct*)\n"
     "declare void @perror(i8*)\n"
     "declare i32 @posix_memalign(i8**, i64, i64)\n"
     "declare double @pow(double, double)\n"
     "declare float @powf(float, float)\n"
     "declare x86_fp80 @powl(x86_fp80, x86_fp80)\n"
     "declare i32 @printf(i8*, ...)\n"
+    "declare %struct @pthread_self()\n"
     "declare i32 @putc(i32, %struct*)\n"
     "declare i32 @putchar(i32)\n"
     "declare i32 @puts(i8*)\n"
     "declare void @qsort(i8*, i64, i64, i32 (i8*, i8*)*)\n"
     "declare i64 @readlink(i8*, i8*, i64)\n"
     "declare i8* @realloc(i8*, i64)\n"
     "declare i8* @reallocf(i8*, i64)\n"
     "declare i32 @remove(i8*)\n"