[PATCH] D132352: Introduce noread_thread_id to address the thread identification problem in coroutines

[John McCall via Phabricator via llvm-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/CodeGen/CGStmt.cpp:2260
       Result.addFnAttr(llvm::Attribute::ReadNone);
-    else if (ReadOnly)
+      Result.addFnAttr(llvm::Attribute::NoReadThreadID);
+    } else if (ReadOnly)
----------------
Hmm, my comment here got lost somehow.

This looks like a new semantic assumption.  Can we split this patch so that this is separate from the introduction of the new attribute?  I know we were assuming this until a few weeks ago, but still, it's generally best practice to do representation changes independent from semantic changes.

Also, this is basically saying that relying on the thread identity is a side-effect that needs to be reflected in the constraints.  Is there documentation justifying that assumption?  Doesn't this make some kinds of existing code invalid?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132352/new/

https://reviews.llvm.org/D132352