[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

[Sameer Sahasrabuddhe via Phabricator via cfe-commits]

sameerds added inline comments.


================
Comment at: clang/test/CodeGenHIP/builtin_memory_fence.cpp:9
+  // CHECK: fence syncscope("workgroup") seq_cst
+  __builtin_memory_fence(__ATOMIC_SEQ_CST,  "workgroup");
+  
----------------
JonChesterfield wrote:
> JonChesterfield wrote:
> > sameerds wrote:
> > > JonChesterfield wrote:
> > > > sameerds wrote:
> > > > > JonChesterfield wrote:
> > > > > > saiislam wrote:
> > > > > > > sameerds wrote:
> > > > > > > > Orderings like `__ATOMIC_SEQ_CST` are defined for C/C++ memory models. They should not be used with the new builtin because this new builtin does not follow any specific language model. For user convenience, the right thing to do is to introduce new tokens in the Clang preprocessor, similar to the `__ATOMIC_*` tokens. The convenient shortcut is to just tell the user to supply numerical values by looking at the LLVM source code.
> > > > > > > > 
> > > > > > > > From llvm/Support/AtomicOrdering.h, note how the numerical value for `__ATOMIC_SEQ_CST` is 5, but the numerical value for the LLVM SequentiallyConsistent ordering is 7. The numerical value 5 refers to the LLVM ordering "release". So, if the implementation were correct, this line should result in the following unexpected LLVM IR:
> > > > > > > >   fence syncscope("workgroup") release
> > > > > > > As you pointed out, the range of acquire to sequentiallly consistent memory orders for llvm::AtomicOrdering is [4, 7], while for llvm::AtomicOrderingCABI is [2, 5]. Enums of C ABI was taken to ensure easy of use for the users who are familiar with C/C++ standard memory model. It allows them to use macros like __ATOMIC_ACQUIRE etc.
> > > > > > > Clang CodeGen of the builtin internally maps C ABI ordering to llvm atomic ordering.
> > > > > > What language, implemented in clang, do you have in mind that reusing the existing __ATOMIC_* macros would be incorrect for?
> > > > > I think we agreed that this builtin exposes the LLVM fence exactly. That would mean it takes arguments defined by LLVM. If you are implementing something different from that, then it first needs to be specified properly. Perhaps you could say that this is a C ABI compatible builtin, that happens to take target specific scopes? That should cover OpenCL whose scope enum is designed to be compatible with C.
> > > > > 
> > > > > Whatever it is that you are trying to implement here, it definitely does not expose a raw LLVM fence.
> > > > The llvm fence, in text form, uses a symbol for the memory scope. Not an enum.
> > > > 
> > > > This symbol is set using these macros for the existing atomic builtins. Using an implementation detail of clang instead is strictly worse, by layering and by precedent.
> > > > 
> > > > ABI is not involved here. Nor is OpenCl.
> > > The `__ATOMIC_*` symbols in Clang quite literally represent the C/C++ ABI. See the details in AtomicOrdering.h and InitPreprocessor.cpp. I am not opposed to specifying that the builtin expects these symbols, but then it is incorrect to say that the builtin exposes the raw LLVM builtin. It is a C-ABI-compatible builtin that happens to take target-specific scope as a string argument. And that would also make it an overload of the already existing builting __atomic_fence().
> > I don't know what you mean by "raw",  but am guessing you're asking for documentation for the intrinsic. Said documentation should indeed be added for this builtin - it'll probably be in a tablegen file.
> I will try to stop using builtin and intrinsic as synonyms.
Right. It's actually an LLVM instruction, not even an intrinsic. I am guilty of using the wrong term quite often, but usually the context helps. I think the following is still needed:

  # A testcase that exercises the builtin with an invalid string argument for scope.
  # An update to the change description describing what is being introduced here. It is more than a direct mapping from builtin to instruction. The C ABI is involved.
  # An update to the Clang documentation describing this new builtin: https://clang.llvm.org/docs/LanguageExtensions.html#builtin-functions



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75917