[Coroutines] Don't optimize readnone function before we split coroutine (4/5)

[ChuanqiXu]

Summary

The revision tries to fix https://github.com/llvm/llvm-project/issues/47177. The key reason for this bug is similar to https://github.com/llvm/llvm-project/issues/47179. It is about the introduction of coroutine breaks the assumption that a function could only be executed in one thread. Also the fix is similar too. I add a wrapper for readnone function to block the optimizations. The main difference is that we missed optimization chances for readnone function if we enabled coroutine. And this would be addressed in following patch.

[ChuanqiXu]

Emit coro_readnone attribute in the frontend.

[rjmccall]

clang/lib/CodeGen/CGCall.cpp

2132

This is a global setting and will affect every function in every file that has coroutines enabled, which is presumably every file compiled with -std=c++20 or later. So you're radically changing optimization for a ton of C++ code that doesn't use coroutines.

It should be fine to just add coro_readnone instead of readnone to call sites from coroutine bodies, right? Oh, but I guess we could have a readnone call in an inlined function body. Can we just suppress this kind of code motion in coroutine bodies, or is that too invasive to the optimizer?

Actually, maybe your change to this patch was a misunderstanding. What I was trying to say in Discourse was that I wasn't sure that your early pass approach would work for thread-local variables, because addresses of thread-locals are LLVM constants and can get moved around implicitly; you need the frontend to be involved there so that there's something hooked into the right place in the emitted function. But I think it should work for this readnone annotation.

[ChuanqiXu]

Emit coro_readnone in CoroEarly pass instead of frontend.

[ChuanqiXu]

clang/lib/CodeGen/CGCall.cpp

2132

Actually, maybe your change to this patch was a misunderstanding. What I was trying to say in Discourse was that I wasn't sure that your early pass approach would work for thread-local variables, because addresses of thread-locals are LLVM constants and can get moved around implicitly; you need the frontend to be involved there so that there's something hooked into the right place in the emitted function. But I think it should work for this readnone annotation.

Yeah... I misunderstood your words. You were talking about TLS variable only but I thought you mentioned both.

This is a global setting and will affect every function in every file that has coroutines enabled, which is presumably every file compiled with -std=c++20 or later. So you're radically changing optimization for a ton of C++ code that doesn't use coroutines.

Yeah... it is a little better in current version since it affects codes with coroutines only. So it wouldn't affect codes compiled with -std=c++20 or later if they don't use coroutines. For optimizations, I was imaging we could re-enable the optimization by running EarlyCSE pass after CoroCleanup pass in https://reviews.llvm.org/D124364. But as you said, the optimization would be changed in the proposal.

Can we just suppress this kind of code motion in coroutine bodies, or is that too invasive to the optimizer?

Yes. According to the previous discussion (https://discourse.llvm.org/t/rfc-coroutine-and-pthread-self/56985), people don't like the direction to insert checks for coroutines around the passes. It is a burden for other developers to understand and remember they may handle a coroutine.

It should be fine to just add coro_readnone instead of readnone to call sites from coroutine bodies, right? Oh, but I guess we could have a readnone call in an inlined function body.

I am trying to do that. I think we could replace coro_readnone with readnone when we are inlining a call into an unlowered coroutine. However, I meet a technical problem that it doesn't work if we don't remove readnone for the function declaration...
I would mark this patch as plan changes before I could get a conclusion if it is possible.

---

I think we could go to https://reviews.llvm.org/D124361 first, it doesn't depend on this one and it wouldn't meet the optimization changing problem.

[ChuanqiXu]

clang/lib/CodeGen/CGCall.cpp

2132

replace coro_readnone with readnone

replace readnone with coro_readnone

[ChuanqiXu]

https://reviews.llvm.org/D125292

clang/lib/CodeGen/CGCall.cpp

if (const FunctionDecl *Fn = dyn_cast<FunctionDecl>(TargetDecl)) {
       // Only place nomerge attribute on call sites, never functions. This
       // allows it to work on indirect virtual function calls.
       if (AttrOnCallSite && TargetDecl->hasAttr<NoMergeAttr>())
         FuncAttrs.addAttribute(llvm::Attribute::NoMerge);
     }
 
     // 'const', 'pure' and 'noalias' attributed functions are also nounwind.
     if (TargetDecl->hasAttr<ConstAttr>()) {
+      if (!LangOpts.Coroutines)
         FuncAttrs.addAttribute(llvm::Attribute::ReadNone);
+      else
+        // This is a workaround for the bug about pthread_self() in coroutine.
+        // See https://github.com/llvm/llvm-project/issues/47177 for the
+        // background. The reason behind the bug is that pthread_self() is
+        // marked as
+        // __attribute__((__const__)) which would be converted to `readnone`
+        // when get lowered to LLVM IR. The readnone function without parameters
+        // implies that all the calls to the function would return the same
+        // result. So the compiler would optimize the following code:
+        //
+        //   auto a = pthread_self();
+        //   co_await something();
+        //   auto b = pthread_self();
+        //
+        // to
+        //
+        //   auto a = pthread_self();
+        //   co_await something();
+        //   // replace uses of b with a
+        //
+        // The transformation is incorrect in case the coroutine might resume in
+        // another thread. The key reason for the bug is the abuse of
+        // __attribute__((__const__)) for thread identification. However, it is
+        // not easy to fix the problem in the library side (we don't know if
+        // there are other similar problems in other places) and ask the end
+        // user to update the library. So we choose to block the optimization
+        // before we split coroutine by replacing readnone attribute as a
+        // placeholder so that we could rewrite readnone after we lowered
+        // coroutines.
+        FuncAttrs.addAttribute(llvm::Attribute::CoroReadNone);
+
       FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
       // gcc specifies that 'const' functions have greater restrictions than
       // 'pure' functions, so they also cannot have infinite loops.
       FuncAttrs.addAttribute(llvm::Attribute::WillReturn);
     } else if (TargetDecl->hasAttr<PureAttr>()) {
       FuncAttrs.addAttribute(llvm::Attribute::ReadOnly);
       FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
       // gcc specifies that 'pure' functions cannot have infinite loops.

clang/test/CodeGenCoroutines/coro-readnone.cpp

+// This tests that
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 -O3 -emit-llvm %s -o - | FileCheck %s
+
+#include "Inputs/coroutine.h"
+
+struct awaitable {
+  bool await_ready() { return false; }
+  void await_suspend(std::coroutine_handle<> h);
+  void await_resume() {}
+};
+awaitable switch_to_new_thread();
+
+struct task {
+  struct promise_type {
+    task get_return_object() { return {}; }
+    std::suspend_never initial_suspend() { return {}; }
+    std::suspend_never final_suspend() noexcept { return {}; }
+    void return_void() {}
+    void unhandled_exception() {}
+  };
+};
+
+__attribute__((const)) int pthread_self_mock();
+
+void check(int i, int j);
+
+task resuming_on_new_thread() {
+  auto i = pthread_self_mock();
+  co_await switch_to_new_thread();
+  auto j = pthread_self_mock();
+  if (i != j)
+    check(i, j);
+}
+
+// CHECK-LABEL: define internal fastcc void @_Z22resuming_on_new_threadv.resume
+// CHECK:         %[[RELOAD:.+]] = load i32, ptr %[[RELOAD_ADDR:.+reload.addr.*]]
+// CHECK:         %[[CALL:.+]] = tail call noundef i32 @_Z17pthread_self_mockv() #[[CONST_ATTR:.+]]
+// CHECK:         %[[CMP:.+]] = icmp eq i32 %[[RELOAD]], %[[CALL]]
+// CHECK:         tail call void @_Z5checkii({{.*}}%[[RELOAD]], {{.*}}%[[CALL]])
+// CHECK:         attributes #[[CONST_ATTR]] = {{.*}}coro_readnone

llvm/docs/LangRef.rst

``readnone``
     On an argument, this attribute indicates that the function does not
     dereference that pointer argument, even though it may read or write the
     memory that the pointer points to if accessed through other pointers.
 
     If a readnone function reads or writes memory visible outside the function,
     or has other side-effects, the behavior is undefined. If a
     function reads from or writes to a readnone pointer argument, the behavior
     is undefined.
+``coro_readnone``
+    On a function, this attribute indicates that the function computes its
+    result (or decides to unwind an exception) based strictly on its arguments
+    and the thread identity. The frontend should emit this attribute only if
+    coroutines are enabled. Otherwise, ``readnone`` should be used. In case
+    coroutines are enabled, the ``coro_readnone`` would be converted to ``readnone``
+    after coruotines get lowered.
 ``readonly``
     On a function, this attribute indicates that the function does not write
     through any pointer arguments (including ``byval`` arguments) or otherwise
     modify any state (e.g. memory, control registers, etc) visible outside the
     ``readonly`` function. It may dereference pointer arguments and read
     state that may be set in the caller. A readonly function always
     returns the same value (or unwinds an exception identically) when
     called with the same set of arguments and global state.  This means while it

llvm/lib/Transforms/Coroutines/CoroEarly.cpp

return coro::declaresIntrinsics(
           "llvm.coro.suspend"});
 }
 
 PreservedAnalyses CoroEarlyPass::run(Module &M, ModuleAnalysisManager &) {
   if (!declaresCoroEarlyIntrinsics(M))
     return PreservedAnalyses::all();
 
   Lowerer L(M);
-  for (auto &F : M) {
+  for (auto &F : M)
     L.lowerEarlyIntrinsics(F);
 
   PreservedAnalyses PA;
   PA.preserveSet<CFGAnalyses>();
   return PA;
 }

llvm/test/Transforms/Coroutines/coro-readnone-01.ll

-; Tests that the readnone function attribute could be lowered correctly by
-; CoroEarly pass.
-; RUN: opt < %s -S -passes=coro-early -opaque-pointers | FileCheck %s
+; Tests that the readnone function which cross suspend points wouldn't misoptimized
+; RUN: opt < %s -S -passes='default<O3>' -opaque-pointers | FileCheck %s
 
 define ptr @f() "coroutine.presplit" {
 entry:
   %id = call token @llvm.coro.id(i32 0, i8* null, i8* null, i8* null)
   %size = call i32 @llvm.coro.size.i32()
   %alloc = call i8* @malloc(i32 %size)
   %hdl = call i8* @llvm.coro.begin(token %id, i8* %alloc)
   %j = call i32 @readnone_func() readnone
   %sus_result = call i8 @llvm.coro.suspend(token none, i1 false)
   switch i8 %sus_result, label %suspend [i8 0, label %resume
                                          i8 1, label %cleanup]
 resume:
-  %i = call i32 @readnone_func() readnone
+  %i = call i32 @readnone_func() coro_readnone
   %cmp = icmp eq i32 %i, %j
   br i1 %cmp, label %same, label %diff
 
 same:
   call void @print_same()
   br label %cleanup
 
 diff:
   call void @print_diff()
   br label %cleanup
 
 cleanup:
   %mem = call i8* @llvm.coro.free(token %id, i8* %hdl)
   call void @free(i8* %mem)
   br label %suspend
 
 suspend:
   call i1 @llvm.coro.end(i8* %hdl, i1 0)
   ret i8* %hdl
 }
 
-; CHECK: %j = call i32 @readnone_func()
-; CHECK: %i = call i32 @readnone_func()
-; CEHCK: declare i32 @readnone_func() #[[ATTR_NUM:[0-9]+]]
-; attributes #[[ATTR_NUM]] = { coro_readnone }
+; CHECK-LABEL: f.resume(
+; CHECK: br i1 %cmp, label %same, label %diff
+; CHECK-EMPTY:
+; CHECK-NEXT: same:
+; CHECK-NEXT:   call void @print_same()
+; CHECK-NEXT:   br label
+; CHECK-EMPTY:
+; CHECK-NEXT: diff:
+; CHECK-NEXT:   call void @print_diff()
+; CHECK-NEXT:   br label
 
-declare i32 @readnone_func() readnone
+declare i32 @readnone_func() coro_readnone
 
 declare void @print_same()
 declare void @print_diff()
 declare i8* @llvm.coro.free(token, i8*)
 declare i32 @llvm.coro.size.i32()
 declare i8  @llvm.coro.suspend(token, i1)
 
 declare token @llvm.coro.id(i32, i8*, i8*, i8*)
 declare i1 @llvm.coro.alloc(token)
 declare i8* @llvm.coro.begin(token, i8*)
 declare i1 @llvm.coro.end(i8*, i1)
 
 declare noalias i8* @malloc(i32)
 declare void @free(i8*)

llvm/test/Transforms/Coroutines/coro-readnone-02.ll

 suspend:                                          ; preds = %cleanup, %entry
   %0 = call i1 @llvm.coro.end(ptr %hdl, i1 false) #5
   ret ptr %hdl
 }
 
 ; CHECK: %j = call i32 @readnone_func() #[[ATTR_NUM:[0-9]+]]
 ; CHECK: %i = call i32 @readnone_func() #[[ATTR_NUM]]
 ; CEHCK: declare i32 @readnone_func() #[[ATTR_NUM]]
-; attributes #[[ATTR_NUM]] = { coro_readnone }
+; CHECK: attributes #[[ATTR_NUM]] = { readnone }
 
 declare i32 @readnone_func() coro_readnone
 declare void @print_same()
 declare void @print_diff()
 
 declare ptr @llvm.coro.free(token, ptr nocapture readonly)
 declare i32 @llvm.coro.size.i32()
 declare i8 @llvm.coro.suspend(token, i1)
 declare token @llvm.coro.id(i32, ptr readnone, ptr nocapture readonly, ptr)
 declare i1 @llvm.coro.alloc(token)
 declare ptr @llvm.coro.begin(token, ptr writeonly)
 declare i1 @llvm.coro.end(ptr, i1)
 declare noalias ptr @malloc(i32)
 declare void @free(ptr)

llvm/test/Transforms/Coroutines/coro-readnone-03.ll

-; Tests that the readnone function which cross suspend points wouldn't misoptimized
-; RUN: opt < %s -S -passes='default<O3>' -opaque-pointers | FileCheck %s
-
-define ptr @f() "coroutine.presplit" {
-entry:
-  %id = call token @llvm.coro.id(i32 0, i8* null, i8* null, i8* null)
-  %size = call i32 @llvm.coro.size.i32()
-  %alloc = call i8* @malloc(i32 %size)
-  %hdl = call i8* @llvm.coro.begin(token %id, i8* %alloc)
-  %j = call i32 @readnone_func() readnone
-  %sus_result = call i8 @llvm.coro.suspend(token none, i1 false)
-  switch i8 %sus_result, label %suspend [i8 0, label %resume
-                                         i8 1, label %cleanup]
-resume:
-  %i = call i32 @readnone_func() readnone
-  %cmp = icmp eq i32 %i, %j
-  br i1 %cmp, label %same, label %diff
-
-same:
-  call void @print_same()
-  br label %cleanup
-
-diff:
-  call void @print_diff()
-  br label %cleanup
-
-cleanup:
-  %mem = call i8* @llvm.coro.free(token %id, i8* %hdl)
-  call void @free(i8* %mem)
-  br label %suspend
-
-suspend:
-  call i1 @llvm.coro.end(i8* %hdl, i1 0)
-  ret i8* %hdl
-}
-
-; CHECK-LABEL: f.resume(
-; CHECK: br i1 %cmp, label %same, label %diff
-; CHECK-EMPTY:
-; CHECK-NEXT: same:
-; CHECK-NEXT:   call void @print_same()
-; CHECK-NEXT:   br label
-; CHECK-EMPTY:
-; CHECK-NEXT: diff:
-; CHECK-NEXT:   call void @print_diff()
-; CHECK-NEXT:   br label
-
-declare i32 @readnone_func() readnone
-
-declare void @print_same()
-declare void @print_diff()
-declare i8* @llvm.coro.free(token, i8*)
-declare i32 @llvm.coro.size.i32()
-declare i8  @llvm.coro.suspend(token, i1)
-
-declare token @llvm.coro.id(i32, i8*, i8*, i8*)
-declare i1 @llvm.coro.alloc(token)
-declare i8* @llvm.coro.begin(token, i8*)
-declare i1 @llvm.coro.end(i8*, i1)
-
-declare noalias i8* @malloc(i32)
-declare void @free(i8*)