[PATCH] D150985: [clang] Allow fp in atomic fetch max/min builtins

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 4 inline comments as done.
yaxunl added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:6576-6578
       if (!ValType->isFloatingType())
         return false;
+      if (!(AllowedType & AOAVT_FP))
----------------
tra wrote:
> Collapse into a single if statement: `if (!(ValType->isFloatingType() && (AllowedType & AOAVT_FP)))`
will do


================
Comment at: clang/lib/Sema/SemaChecking.cpp:6588
+    if (!IsAllowedValueType(ValType, ArithAllows)) {
+      assert(ArithAllows & AOAVT_Integer);
+      auto DID = ArithAllows & AOAVT_FP
----------------
tra wrote:
> Why do we expect a failed `IsAllowedValueType` check to fail only if we were allowed integers? Is that because we assume that all atomic instructions support integers?
> 
> If that's the case, I'd hoist the assertion and apply it right after we're done setting `ArithAllows`. Alternatively, we could discard `AOAVT_Integer` and call the enum `ArithOpExtraValueType`. Tracking a bit that's always set does not buy us much, though it does make the code a bit more uniform. Up to you.
> 
> 
> 
we assume integer is always allowed. AOAVT_Integer is for uniformity. I will change the enum to ArithOpExtraValueType and remove AOAVT_Integer 


================
Comment at: clang/test/Sema/atomic-ops.c:134
        int *I, const int *CI,
        int **P, float *D, struct S *s1, struct S *s2) {
   __c11_atomic_init(I, 5); // expected-error {{pointer to _Atomic}}
----------------
tra wrote:
> I wonder why we have this inconsistency in the non-atomic arguments.
> We don't actually have any double variants and the argument `D` is actually a `float *`, even though the naming convention used suggests that it should've been either a `double *` or should be called `F`.
> 
will rename it to F and add double *D


================
Comment at: clang/test/Sema/atomic-ops.c:218
   __atomic_fetch_sub(s1, 3, memory_order_seq_cst); // expected-error {{must be a pointer to integer, pointer or supported floating point type}}
-  __atomic_fetch_min(D, 3, memory_order_seq_cst); // expected-error {{must be a pointer to integer}}
-  __atomic_fetch_max(P, 3, memory_order_seq_cst); // expected-error {{must be a pointer to integer}}
+  __atomic_fetch_min(D, 3, memory_order_seq_cst);
+  __atomic_fetch_max(P, 3, memory_order_seq_cst); // expected-error {{must be a pointer to integer or supported floating point type}}
----------------
tra wrote:
> We seem to be missing the tests for `double *` here, too.
will add it


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

https://reviews.llvm.org/D150985