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

[Jon Chesterfield via Phabricator via cfe-commits]

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 argument which defines what synchronizes-with edges they add. They can only be given acquire, release, acq_rel, and seq_cst orderings.




================
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;
----------------
isIntegerType will return true for signed integers as well as unsigned. It seems reasonable to call this with a signed integer type (e.g. '2'), so perhaps the references to unsigned should be dropped from the code and error message



================
Comment at: clang/lib/Sema/SemaChecking.cpp:1889
+
+    // Check if Order is one of the valid types
+    if (!llvm::isValidAtomicOrderingCABI(ord)) {
----------------
This should reject 'relaxed' - I think that's currently accepted by sema then silently thrown away by codegen


================
Comment at: clang/test/CodeGenOpenCL/atomic-ops.cl:291
 
+void test_memory_fence() {
+  // CHECK-LABEL: @test_memory_fence
----------------
I'm hoping this intrinsic will be usable from C or C++, as well as from OpenCL - please could you add a non-opencl codegen test.

It doesn't need to check all the cases again, just enough to show that the intrinsic and arguments are available (they're spelled like `__ATOMIC_SEQ_CST`, `__OPENCL_MEMORY_SCOPE_ALL_SVM_DEVICES` outside of opencl)


================
Comment at: clang/test/SemaOpenCL/atomic-ops.cl:198
+
+void memory_fence_errors() {
+  __builtin_memory_fence(memory_order_seq_cst + 1, memory_scope_work_group); // expected-error {{memory order argument to fence operation is invalid}}
----------------
A happy case too please, e.g. to show that it accepts a couple of integers. Looks like ` __builtin_memory_fence(2, 2);` but without an expected-error comment


Repository:
  rC Clang

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

https://reviews.llvm.org/D75917