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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
