[PATCH] D21723: [RFC] Enhance synchscope representation

[Tony Tye via Phabricator via llvm-commits]

t-tye added inline comments.


================
Comment at: docs/LangRef.rst:2173
+example, OpenCL supports separate memory scopes for device, work-group and
+sub-group).
+
----------------
kzhuravl wrote:
> mehdi_amini wrote:
> > I'm not convinced by the wording `it is target defined how ``<n>`` affects which other operations`, it can be read that even with the same scope it is still target define, which is not the intent right?
> > 
> > Straw man alternative:
> > 
> > ```
> > If an atomic operation is marked ``syncscope(<n>)``, then it *synchronizes with*,
> > and participates in the seq\_cst total orderings of, other operations
> > running in the same scope. It is target defined how it interacts with operations
> > in a different scope.
> > ```
> > 
> Yes, you are right. I have updated the wording.
In OpenCL, memory scopes define scope "levels" which correspond to how threads are grouped for execution. For example, a set of threads that belong to the same work-group, and specify the "workgroup" scope, only synchronize with threads in the same work-group instance. Threads in a different work-group instance do not synchronize even if they specify the "workgroup" scope.

So in general, if the scopes are target specific, it is also target specific how the the threads are grouped, and how scopes are related (for example inclusive).

I was also curious what the definition of singlethread is. My understanding was that synchronization only happens between atomic operations. So does both the signal handler and the non-signal handler code have to have atomic operations using singlethread? If used in a fence, what atomic operations pair with the fence to create the synchronizes-with relation? Generally non-atomic operations would not pair. The wording seems to imply a singlethread atomic is synchronizing with any operations in the same thread, and is presumably using thread to mean OS thread, rather than memory model notion of thread of execution which would have the signal handler and non signal handler code execution in different threads.

Since this text is fairly informal, how correct does it need to be?


================
Comment at: docs/LangRef.rst:7295
       <result> = load [volatile] <ty>, <ty>* <pointer>[, align <alignment>][, !nontemporal !<index>][, !invariant.load !<index>][, !invariant.group !<index>][, !nonnull !<index>][, !dereferenceable !<deref_bytes_node>][, !dereferenceable_or_null !<deref_bytes_node>][, !align !<align_node>]
-      <result> = load atomic [volatile] <ty>, <ty>* <pointer> [singlethread] <ordering>, align <alignment> [, !invariant.group !<index>]
+      <result> = load atomic [volatile] <ty>, <ty>* <pointer> [syncscope("<a>")] <ordering>, align <alignment> [, !invariant.group !<index>]
       !<index> = !{ i32 1 }
----------------
Suggest [syncscope("<scope>")] to be consistent with other arguments. Same comment for other uses below.


================
Comment at: lib/CodeGen/MIRParser/MIParser.cpp:2082
+      return error("expected '(' in syncscope");
+    if (!Token.is(MIToken::Identifier))
+      return error("expected identifier in syncscope");
----------------
To be consistent with the LLVM IR syntax should this use a quoted string rather than an identified? Would need to add a MIToken::dquote and then consumeIfPresent(MIToken::dquote) before/after.


================
Comment at: lib/CodeGen/MIRPrinter.cpp:1032
+      Context.getSyncScopeNames(SSNs);
+    OS << "syncscope(" << SSNs[SSID] << ") ";
+    break;
----------------
To be consistent with the LLVM IR syntax should this be a quoted string?


================
Comment at: lib/IR/Core.cpp:2747
 LLVMValueRef LLVMBuildFence(LLVMBuilderRef B, LLVMAtomicOrdering Ordering,
                             LLVMBool isSingleThread, const char *Name) {
   return wrap(
----------------
Should this and the other atomic instruction support building with a syncscope?


https://reviews.llvm.org/D21723