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

This revision was automatically updated to reflect the committed changes. Closed by commit rG06bdffb2bb45: [AMDGPU] Expose llvm fence instruction as clang intrinsic (authored by saiislam, committed by sameerds). Changed prior to commit: https://reviews.llvm.org/D75917?vs=260053=260207#toc

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

saiislam updated this revision to Diff 260053. saiislam added a comment. Updated description and added a failing test case for integer scope. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75917/new/ https://reviews.llvm.org/D75917 Files:

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

sameerds accepted this revision. sameerds added a comment. This revision is now accepted and ready to land. Thanks @saiislam ... this looks much better! Two nitpicks, that must be fixed. But it is okay if you directly submit after fixing them. 1. The change description should use "const char

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

saiislam marked an inline comment as done. saiislam added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:2958 + // Check that sync scope is a constant literal + if(!ArgExpr->EvaluateAsConstantExpr(ArgResult1, + Expr::EvaluateForCodeGen, Context))

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

saiislam updated this revision to Diff 259545. saiislam added a comment. Added check and test for sync scope to be a constant literal. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75917/new/ https://reviews.llvm.org/D75917 Files:

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

saiislam updated this revision to Diff 259485. saiislam added a comment. Removed documentation from clang doc. Squashed all changes into a single commit. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75917/new/ https://reviews.llvm.org/D75917

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

sameerds added a comment. Can you please submit a squashed single commit for review against the master branch? All the recent commits seem to be relative to previous commits, and I am having trouble looking at the change as a whole. The commit description needs some fixes: 1. Remove use of

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

saiislam marked an inline comment as done. saiislam added inline comments. Comment at: clang/test/CodeGenCXX/builtin-amdgcn-fence-failure.cpp:5 + +void test_amdgcn_fence_failure() { + b-sumner wrote: > JonChesterfield wrote: > > arsenm wrote: > > > Does this

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

saiislam updated this revision to Diff 259298. saiislam added a comment. Moved builtin-amdgcn-fence-failure.cpp from clang/test/CodeGenCXX/ to clang/test/Sema/ since it is checking an error. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75917/new/

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

b-sumner added inline comments. Comment at: clang/test/CodeGenCXX/builtin-amdgcn-fence-failure.cpp:5 + +void test_amdgcn_fence_failure() { + JonChesterfield wrote: > arsenm wrote: > > Does this really depend on C++? Can it use OpenCL like the other builtin > >

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

JonChesterfield added inline comments. Comment at: clang/test/CodeGenCXX/builtin-amdgcn-fence-failure.cpp:5 + +void test_amdgcn_fence_failure() { + arsenm wrote: > Does this really depend on C++? Can it use OpenCL like the other builtin > tests?This also

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

arsenm added inline comments. Comment at: clang/test/CodeGenCXX/builtin-amdgcn-fence-failure.cpp:5 + +void test_amdgcn_fence_failure() { + Does this really depend on C++? Can it use OpenCL like the other builtin tests?This also belongs in a Sema* test directory

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

JonChesterfield added a comment. Amdgcn specific is fine by me. Hopefully that unblocks this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75917/new/ https://reviews.llvm.org/D75917 ___ cfe-commits

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

saiislam updated this revision to Diff 259239. saiislam added a comment. Removed stale commented code Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75917/new/ https://reviews.llvm.org/D75917 Files: clang/docs/LanguageExtensions.rst

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

saiislam updated this revision to Diff 259238. saiislam added a comment. Herald added subscribers: kerbowa, nhaehnle, jvesely. Changed the builtin to be AMDGCN-specific It is named as __builtin_amdgcn_fence(order, scope) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

JonChesterfield added a comment. The tests look good, but I can't see the implementation in this diff. Maybe a file missing from the patch? Can be hard to tell with phabricator, the error may be at my end. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

saiislam updated this revision to Diff 258368. saiislam marked an inline comment as done. saiislam added a comment. 1. Moved test case to clang/test/CodeGenCXX. 2. Added a failing test case with invalid sync scope, which gets detected by implementation of fence instruction. 3. Updated the change

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

b-sumner added a comment. In addition to predefining `__ATOMIC_RELAXED`, etc., clang also predefines `__OPENCL_MEMORY_SCOPE_WORK_ITEM` and friends. So it doesn't really seem unreasonable for clang to also predefine its known syncscopes, and to require the argument to be one of those integers.

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

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: > >

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

JonChesterfield added inline comments. Comment at: clang/test/CodeGenHIP/builtin_memory_fence.cpp:1 // REQUIRES: amdgpu-registered-target // RUN: %clang_cc1 %s -x hip -emit-llvm -O0 -o - \ Codegen test should be under CodeGen and/or CodeGenCXX Repository:

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

JonChesterfield 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: > sameerds wrote: > >

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

JonChesterfield 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"); + sameerds wrote: > JonChesterfield wrote: > >

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

sameerds requested changes to this revision. sameerds added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13651 + llvm::getConstantStringInfo(Scope, scp); + SSID =

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

JonChesterfield added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13651 + llvm::getConstantStringInfo(Scope, scp); + SSID = getLLVMContext().getOrInsertSyncScopeID(scp); + sameerds wrote: > saiislam wrote: > > sameerds wrote: > > >

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

sameerds added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13651 + llvm::getConstantStringInfo(Scope, scp); + SSID = getLLVMContext().getOrInsertSyncScopeID(scp); + saiislam wrote: > sameerds wrote: > > This seems to be creating a new

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

saiislam updated this revision to Diff 255288. saiislam added a comment. Changes: 1. Moved builtin definition with rest of atomic builtins 2. Updated validity checking of memory order using exact mathches instead of range matching 3. Added a sucessful test case which passes arbitrary string

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

JonChesterfield added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:1888 +// Check valididty of memory ordering as per C11 / C++11's memody model. +if (ord < static_cast(llvm::AtomicOrderingCABI::acquire) || + ord >

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

saiislam marked 5 inline comments as done. saiislam added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13630 + + // Map C11/C++11 memory ordering to LLVM memory ordering + switch (static_cast(ord)) { sameerds wrote: > There should no

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

sameerds requested changes to this revision. sameerds added inline comments. This revision now requires changes to proceed. Comment at: clang/include/clang/Basic/Builtins.def:1583 +// Second argument : target specific sync scope string +BUILTIN(__builtin_memory_fence, "vUicC*",

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

saiislam updated this revision to Diff 255173. saiislam added a comment. Removed OpenCL specific dependencies Now it takes target-specific sync scope as an string. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75917/new/

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

b-sumner added a comment. Please go ahead and update to a string for the scope. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75917/new/ https://reviews.llvm.org/D75917 ___ cfe-commits mailing list

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

JonChesterfield added a comment. Can this be revived? Changing the enum to a string still sounds good to me Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75917/new/ https://reviews.llvm.org/D75917 ___ cfe-commits

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

sameerds added a comment. In D75917#1925700 , @JonChesterfield wrote: > > I think I follow. The syncscope takes a string, therefore the builtin that > maps onto fence should also take a string for that parameter? That's fine by > me. Will help if a

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

JonChesterfield added a comment. In D75917#1925617 , @sameerds wrote: > Is it really? The scope argument of the IR fence is a target-specific string: > http://llvm.org/docs/LangRef.html#syncscope > > The change that I see here is assuming a numerical

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

sameerds added a comment. In D75917#1917296 , @JonChesterfield wrote: > In D75917#1916972 , @sameerds wrote: > > > Well, there is a problem: The LangRef says that scopes are target-defined. > > This change says

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

JonChesterfield added a comment. In D75917#1916972 , @sameerds wrote: > Well, there is a problem: The LangRef says that scopes are target-defined. > This change says that scopes are defined by the high-level language and > further assumes that OpenCL

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

sameerds added a comment. In D75917#1916664 , @JonChesterfield wrote: > In D75917#1916160 , @sameerds wrote: > > > how this builtin fits in with the overall scheme of language-specific and > > target-specific

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

JonChesterfield added a comment. In D75917#1916160 , @sameerds wrote: > how this builtin fits in with the overall scheme of language-specific and > target-specific details of an atomic operation. For example, is this meant > only for OpenCL? Does it

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

sameerds added a comment. The commit summary needs improvement. The syntax is not really necessary there, but instead this needs a better explanation of how this builtin fits in with the overall scheme of language-specific and target-specific details of an atomic operation. For example, is

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

Anastasia added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:1880 +// Check if Order is an unsigned +if (!Ty->isIntegerType()) { + Diag(ArgExpr->getExprLoc(), diag::err_typecheck_expect_uint) << Ty; JonChesterfield wrote: >

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

JonChesterfield added a subscriber: jdoerfert. JonChesterfield added a comment. @jdoerfert this is one of the two intrinsics needed to drop the .ll source from the amdgcn deviceRTL. The other is atomic_inc. Repository: rC Clang CHANGES SINCE LAST ACTION

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

JonChesterfield added inline comments. Comment at: clang/include/clang/Basic/Builtins.def:1516 +// Builtin to expose llvm fence instruction +BUILTIN(__builtin_memory_fence, "vUiUi", "t") `BUILTIN(__builtin_memory_fence, "vii", "n")`? The other fence

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

JonChesterfield added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:3713 + switch (ord) { + case 0: // memory_order_relaxed + default: // invalid order Interesting, fence can't be relaxed > ‘fence’ instructions take an ordering

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

saiislam created this revision. saiislam added reviewers: JonChesterfield, sameerds, yaxunl, gregrodgers, b-sumner. Herald added subscribers: cfe-commits, jfb. Herald added a project: clang. "__buitlin_memory_fence(, )" in clang gets mapped to "fence [syncscope("")] ; yields void" in IR.