[Sema] Fix the atomic expr rebuilding order.

[hliao]

Summary

• Rearrange the atomic expr order to the API order when rebuilding atomic expr during template instantiation.

[hliao]

The current BuildAtomicExpr expects the arguments to be in the API order instead of the AST order. If RebuildAtomicExpr uses the same BuildAtomicExpr, it needs to ensure the order of arguments are in API order; otherwise, arguments (especially the one with memory order) will be misplaced.

[erichkeane]

Yikes, good catch!

Would we be better off instead to just modify how the other switch loads the value? Presumably something like, "if (NeedsRearrangeArgs) SubExprs.append(Args.begin(), Args.end()); else /*the switch*/.

clang/lib/Sema/TreeTransform.h

3319 ↗

(On Diff #221363)

When passing a bool, use the comment syntax /*paramname=*/ to make it clear whats happening.

[hliao]

In D67924#1679409, @erichkeane wrote:

Yikes, good catch!

Would we be better off instead to just modify how the other switch loads the value? Presumably something like, "if (NeedsRearrangeArgs) SubExprs.append(Args.begin(), Args.end()); else /*the switch*/.

Loop from L4762 will check "value" arguments assuming the API order as well. That's why arguments are arranged so that the value checking logic also check the correct arguments.

[hliao]

Add parameter name for that default argument.

[erichkeane]

It kinda stinks that we have to do this at such a late step. I'd have much preferred doing this as a part of the rebuild, but it appears that the 'form' takes quite a bit to calculate. Also, I'd likely have preferred that the initial reordering happened in codegen instead of this early, though perhaps thats too large of a change to do here.

I think then we're stuck with this reordering step.

That said, I'm not a fan at all of doing this parameter as a bool, particularly one with an ambiguous name. Can you introduce an enum like "enum class AtomicArgumentOrder { API, AST}" that makes it more clear?

Additionally, ReArgs is a confusing name, how about APIOrderedArgs?

[erichkeane]

Also, can you add a test for GNUCmpXchg in both situations (inside a template, and outside)? It is the most complicated, and would best reflect the issue.

[hliao]

revised

[erichkeane]

Seems your test changes have disappeared as well? Otherwise, 1 comment then I'm ok with this.

clang/lib/Sema/TreeTransform.h

3319 ↗

(On Diff #221369)

The comment is no longer necessary, since the enum describes the purpose.

[hliao]

add test case for compare_exchange.

[hliao]

update

cfe/trunk/include/clang/Sema/Sema.h

public:
   /// locations.
   ExprResult ActOnCallExpr(Scope *S, Expr *Fn, SourceLocation LParenLoc,
                            MultiExprArg ArgExprs, SourceLocation RParenLoc,
                            Expr *ExecConfig = nullptr);
   ExprResult BuildCallExpr(Scope *S, Expr *Fn, SourceLocation LParenLoc,
                            MultiExprArg ArgExprs, SourceLocation RParenLoc,
                            Expr *ExecConfig = nullptr,
                            bool IsExecConfig = false);
-  ExprResult BuildAtomicExpr(SourceRange CallRange, SourceRange ExprRange,
-                             SourceLocation RParenLoc, MultiExprArg Args,
-                             AtomicExpr::AtomicOp Op);
+  enum class AtomicArgumentOrder { API, AST };
+  ExprResult
+  BuildAtomicExpr(SourceRange CallRange, SourceRange ExprRange,
+                  AtomicExpr::AtomicOp Op,
+                  AtomicArgumentOrder ArgOrder = AtomicArgumentOrder::API);
   ExprResult
   BuildResolvedCallExpr(Expr *Fn, NamedDecl *NDecl, SourceLocation LParenLoc,
                         ArrayRef<Expr *> Arg, SourceLocation RParenLoc,
                         Expr *Config = nullptr, bool IsExecConfig = false,
                         ADLCallKind UsesADL = ADLCallKind::NotADL);
 
   ExprResult ActOnCUDAExecConfigExpr(Scope *S, SourceLocation LLLLoc,
                                      MultiExprArg ExecConfig,

cfe/trunk/lib/Sema/SemaChecking.cpp

ExprResult Sema::SemaAtomicOpsOverloaded(ExprResult TheCallResult,
   MultiExprArg Args{TheCall->getArgs(), TheCall->getNumArgs()};
   return BuildAtomicExpr({TheCall->getBeginLoc(), TheCall->getEndLoc()},
                          DRE->getSourceRange(), TheCall->getRParenLoc(), Args,
                          Op);
 }
 
 ExprResult Sema::BuildAtomicExpr(SourceRange CallRange, SourceRange ExprRange,
                                  SourceLocation RParenLoc, MultiExprArg Args,
-                                 AtomicExpr::AtomicOp Op) {
+                                 AtomicExpr::AtomicOp Op,
+                                 AtomicArgumentOrder ArgOrder) {
   // All the non-OpenCL operations take one of the following forms.
   // The OpenCL operations take the __c11 forms with one extra argument for
   // synchronization scope.
   enum {
     // C    __c11_atomic_init(A *, C)
     Init,
 
     // C    __c11_atomic_load(A *, int)
ExprResult Sema::BuildAtomicExpr(SourceRange CallRange, SourceRange ExprRange,
   // arguments are actually passed as pointers.
   QualType ByValType = ValType; // 'CP'
   bool IsPassedByAddress = false;
   if (!IsC11 && !IsN) {
     ByValType = Ptr->getType();
     IsPassedByAddress = true;
   }
 
+  SmallVector<Expr *, 5> APIOrderedArgs;
+  if (ArgOrder == Sema::AtomicArgumentOrder::AST) {
+    APIOrderedArgs.push_back(Args[0]);
+    switch (Form) {
+    case Init:
+    case Load:
+      APIOrderedArgs.push_back(Args[1]); // Val1/Order
+      break;
+    case LoadCopy:
+    case Copy:
+    case Arithmetic:
+    case Xchg:
+      APIOrderedArgs.push_back(Args[2]); // Val1
+      APIOrderedArgs.push_back(Args[1]); // Order
+      break;
+    case GNUXchg:
+      APIOrderedArgs.push_back(Args[2]); // Val1
+      APIOrderedArgs.push_back(Args[3]); // Val2
+      APIOrderedArgs.push_back(Args[1]); // Order
+      break;
+    case C11CmpXchg:
+      APIOrderedArgs.push_back(Args[2]); // Val1
+      APIOrderedArgs.push_back(Args[4]); // Val2
+      APIOrderedArgs.push_back(Args[1]); // Order
+      APIOrderedArgs.push_back(Args[3]); // OrderFail
+      break;
+    case GNUCmpXchg:
+      APIOrderedArgs.push_back(Args[2]); // Val1
+      APIOrderedArgs.push_back(Args[4]); // Val2
+      APIOrderedArgs.push_back(Args[5]); // Weak
+      APIOrderedArgs.push_back(Args[1]); // Order
+      APIOrderedArgs.push_back(Args[3]); // OrderFail
+      break;
+    }
+  } else
+    APIOrderedArgs.append(Args.begin(), Args.end());
+
   // The first argument's non-CV pointer type is used to deduce the type of
   // subsequent arguments, except for:
   //  - weak flag (always converted to bool)
   //  - memory order (always converted to int)
   //  - scope  (always converted to int)
-  for (unsigned i = 0; i != Args.size(); ++i) {
+  for (unsigned i = 0; i != APIOrderedArgs.size(); ++i) {
     QualType Ty;
     if (i < NumVals[Form] + 1) {
       switch (i) {
       case 0:
         // The first argument is always a pointer. It has a fixed type.
         // It is always dereferenced, a nullptr is undefined.
-        CheckNonNullArgument(*this, Args[i], ExprRange.getBegin());
+        CheckNonNullArgument(*this, APIOrderedArgs[i], ExprRange.getBegin());
         // Nothing else to do: we already know all we want about this pointer.
         continue;
       case 1:
         // The second argument is the non-atomic operand. For arithmetic, this
         // is always passed by value, and for a compare_exchange it is always
         // passed by address. For the rest, GNU uses by-address and C11 uses
         // by-value.
         assert(Form != Load);
         if (Form == Init || (Form == Arithmetic && ValType->isIntegerType()))
           Ty = ValType;
         else if (Form == Copy || Form == Xchg) {
-          if (IsPassedByAddress)
+          if (IsPassedByAddress) {
             // The value pointer is always dereferenced, a nullptr is undefined.
-            CheckNonNullArgument(*this, Args[i], ExprRange.getBegin());
+            CheckNonNullArgument(*this, APIOrderedArgs[i],
+                                 ExprRange.getBegin());
+          }
           Ty = ByValType;
         } else if (Form == Arithmetic)
           Ty = Context.getPointerDiffType();
         else {
-          Expr *ValArg = Args[i];
+          Expr *ValArg = APIOrderedArgs[i];
           // The value pointer is always dereferenced, a nullptr is undefined.
           CheckNonNullArgument(*this, ValArg, ExprRange.getBegin());
           LangAS AS = LangAS::Default;
           // Keep address space of non-atomic pointer type.
           if (const PointerType *PtrTy =
                   ValArg->getType()->getAs<PointerType>()) {
             AS = PtrTy->getPointeeType().getAddressSpace();
           }
           Ty = Context.getPointerType(
               Context.getAddrSpaceQualType(ValType.getUnqualifiedType(), AS));
         }
         break;
       case 2:
         // The third argument to compare_exchange / GNU exchange is the desired
         // value, either by-value (for the C11 and *_n variant) or as a pointer.
         if (IsPassedByAddress)
-          CheckNonNullArgument(*this, Args[i], ExprRange.getBegin());
+          CheckNonNullArgument(*this, APIOrderedArgs[i], ExprRange.getBegin());
         Ty = ByValType;
         break;
       case 3:
         // The fourth argument to GNU compare_exchange is a 'weak' flag.
         Ty = Context.BoolTy;
         break;
       }
     } else {
       // The order(s) and scope are always converted to int.
       Ty = Context.IntTy;
     }
 
     InitializedEntity Entity =
         InitializedEntity::InitializeParameter(Context, Ty, false);
-    ExprResult Arg = Args[i];
+    ExprResult Arg = APIOrderedArgs[i];
     Arg = PerformCopyInitialization(Entity, SourceLocation(), Arg);
     if (Arg.isInvalid())
       return true;
-    Args[i] = Arg.get();
+    APIOrderedArgs[i] = Arg.get();
   }
 
   // Permute the arguments into a 'consistent' order.
   SmallVector<Expr*, 5> SubExprs;
   SubExprs.push_back(Ptr);
   switch (Form) {
   case Init:
     // Note, AtomicExpr::getVal1() has a special case for this atomic.
-    SubExprs.push_back(Args[1]); // Val1
+    SubExprs.push_back(APIOrderedArgs[1]); // Val1
     break;
   case Load:
-    SubExprs.push_back(Args[1]); // Order
+    SubExprs.push_back(APIOrderedArgs[1]); // Order
     break;
   case LoadCopy:
   case Copy:
   case Arithmetic:
   case Xchg:
-    SubExprs.push_back(Args[2]); // Order
-    SubExprs.push_back(Args[1]); // Val1
+    SubExprs.push_back(APIOrderedArgs[2]); // Order
+    SubExprs.push_back(APIOrderedArgs[1]); // Val1
     break;
   case GNUXchg:
     // Note, AtomicExpr::getVal2() has a special case for this atomic.
-    SubExprs.push_back(Args[3]); // Order
-    SubExprs.push_back(Args[1]); // Val1
-    SubExprs.push_back(Args[2]); // Val2
+    SubExprs.push_back(APIOrderedArgs[3]); // Order
+    SubExprs.push_back(APIOrderedArgs[1]); // Val1
+    SubExprs.push_back(APIOrderedArgs[2]); // Val2
     break;
   case C11CmpXchg:
-    SubExprs.push_back(Args[3]); // Order
-    SubExprs.push_back(Args[1]); // Val1
-    SubExprs.push_back(Args[4]); // OrderFail
-    SubExprs.push_back(Args[2]); // Val2
+    SubExprs.push_back(APIOrderedArgs[3]); // Order
+    SubExprs.push_back(APIOrderedArgs[1]); // Val1
+    SubExprs.push_back(APIOrderedArgs[4]); // OrderFail
+    SubExprs.push_back(APIOrderedArgs[2]); // Val2
     break;
   case GNUCmpXchg:
-    SubExprs.push_back(Args[4]); // Order
-    SubExprs.push_back(Args[1]); // Val1
-    SubExprs.push_back(Args[5]); // OrderFail
-    SubExprs.push_back(Args[2]); // Val2
-    SubExprs.push_back(Args[3]); // Weak
+    SubExprs.push_back(APIOrderedArgs[4]); // Order
+    SubExprs.push_back(APIOrderedArgs[1]); // Val1
+    SubExprs.push_back(APIOrderedArgs[5]); // OrderFail
+    SubExprs.push_back(APIOrderedArgs[2]); // Val2
+    SubExprs.push_back(APIOrderedArgs[3]); // Weak
     break;
   }
 
   if (SubExprs.size() >= 2 && Form != Init) {
     llvm::APSInt Result(32);
     if (SubExprs[1]->isIntegerConstantExpr(Result, Context) &&
         !isValidOrderingForOp(Result.getSExtValue(), Op))
       Diag(SubExprs[1]->getBeginLoc(),

cfe/trunk/lib/Sema/TreeTransform.h

ExprResult RebuildEmptyCXXFoldExpr(SourceLocation EllipsisLoc,
                                      BinaryOperatorKind Operator) {
     return getSema().BuildEmptyCXXFoldExpr(EllipsisLoc, Operator);
   }
 
   /// Build a new atomic operation expression.
   ///
   /// By default, performs semantic analysis to build the new expression.
   /// Subclasses may override this routine to provide different behavior.
-  ExprResult RebuildAtomicExpr(SourceLocation BuiltinLoc,
-                               MultiExprArg SubExprs,
+  ExprResult RebuildAtomicExpr(SourceLocation BuiltinLoc, MultiExprArg SubExprs,
                                AtomicExpr::AtomicOp Op,
                                SourceLocation RParenLoc) {
     // Use this for all of the locations, since we don't know the difference
     // between the call and the expr at this point.
     SourceRange Range{BuiltinLoc, RParenLoc};
-    return getSema().BuildAtomicExpr(Range, Range, RParenLoc, SubExprs, Op);
+    return getSema().BuildAtomicExpr(Range, Range, RParenLoc, SubExprs, Op,
+                                     Sema::AtomicArgumentOrder::AST);
   }
 
 private:
   TypeLoc TransformTypeInObjectScope(TypeLoc TL,
                                      QualType ObjectType,
                                      NamedDecl *FirstQualifierInScope,
                                      CXXScopeSpec &SS);
 

cfe/trunk/test/AST/atomic-expr.cpp

 // RUN: %clang_cc1 -ast-dump %s | FileCheck %s
 
 template<int N = 0>
 void pr43370() {
   int arr[2];
-  __atomic_store_n(arr, 0, 0);
+  __atomic_store_n(arr, 0, 5);
 }
+
+template<int N = 0>
+void foo() {
+  int arr[2];
+  (void)__atomic_compare_exchange_n(arr, arr, 1, 0, 3, 4);
+}
+
 void useage(){
   pr43370();
+  foo();
 }
 
 // CHECK:FunctionTemplateDecl 0x{{[0-9a-f]+}} <{{[^,]+}}, line:7:1> line:4:6 pr43370
 // CHECK: AtomicExpr
 // CHECK-NEXT: ImplicitCastExpr
 // CHECK-SAME: <ArrayToPointerDecay>
+// CHECK-NEXT: DeclRefExpr 0x{{[0-9a-f]+}} <{{[^:]+}}:20> 'int [2]' lvalue Var 0x{{[0-9a-f]+}} 'arr' 'int [2]'
+// CHECK-NEXT: IntegerLiteral 0x{{[0-9a-f]+}} <{{[^:]+}}:28> 'int' 5
+// CHECK-NEXT: IntegerLiteral 0x{{[0-9a-f]+}} <{{[^:]+}}:25> 'int' 0
 // CHECK:FunctionDecl 0x{{[0-9a-f]+}} <line:4:1, line:7:1> line:4:6 used pr43370
 // CHECK: AtomicExpr
 // CHECK-NEXT: ImplicitCastExpr
 // CHECK-SAME: <ArrayToPointerDecay>
+// CHECK-NEXT: DeclRefExpr 0x{{[0-9a-f]+}} <{{[^:]+}}:20> 'int [2]' lvalue Var 0x{{[0-9a-f]+}} 'arr' 'int [2]'
+// CHECK-NEXT: IntegerLiteral 0x{{[0-9a-f]+}} <{{[^:]+}}:28> 'int' 5
+// CHECK-NEXT: IntegerLiteral 0x{{[0-9a-f]+}} <{{[^:]+}}:25> 'int' 0
+
+// CHECK:FunctionTemplateDecl 0x{{[0-9a-f]+}} <line:9:1, line:13:1> line:10:6 foo
+// CHECK: AtomicExpr
+// CHECK-NEXT: ImplicitCastExpr
+// CHECK-SAME: <ArrayToPointerDecay>
+// CHECK-NEXT: DeclRefExpr 0x{{[0-9a-f]+}} <{{[^:]+}}:37> 'int [2]' lvalue Var 0x{{[0-9a-f]+}} 'arr' 'int [2]'
+// CHECK-NEXT: IntegerLiteral 0x{{[0-9a-f]+}} <{{[^:]+}}:53> 'int' 3
+// CHECK-NEXT: ImplicitCastExpr
+// CHECK-SAME: <ArrayToPointerDecay>
+// CHECK-NEXT: DeclRefExpr 0x{{[0-9a-f]+}} <{{[^:]+}}:42> 'int [2]' lvalue Var 0x{{[0-9a-f]+}} 'arr' 'int [2]'
+// CHECK-NEXT: IntegerLiteral 0x{{[0-9a-f]+}} <{{[^:]+}}:56> 'int' 4
+// CHECK-NEXT: IntegerLiteral 0x{{[0-9a-f]+}} <{{[^:]+}}:47> 'int' 1
+// CHECK-NEXT: ImplicitCastExpr
+// CHECK-NEXT: IntegerLiteral 0x{{[0-9a-f]+}} <{{[^:]+}}:50> 'int' 0
+// CHECK:FunctionDecl 0x{{[0-9a-f]+}} <line:10:1, line:13:1> line:10:6 used foo
+// CHECK: AtomicExpr
+// CHECK-NEXT: ImplicitCastExpr
+// CHECK-SAME: <ArrayToPointerDecay>
+// CHECK-NEXT: DeclRefExpr 0x{{[0-9a-f]+}} <{{[^:]+}}:37> 'int [2]' lvalue Var 0x{{[0-9a-f]+}} 'arr' 'int [2]'
+// CHECK-NEXT: IntegerLiteral 0x{{[0-9a-f]+}} <{{[^:]+}}:53> 'int' 3
+// CHECK-NEXT: ImplicitCastExpr
+// CHECK-SAME: <ArrayToPointerDecay>
+// CHECK-NEXT: DeclRefExpr 0x{{[0-9a-f]+}} <{{[^:]+}}:42> 'int [2]' lvalue Var 0x{{[0-9a-f]+}} 'arr' 'int [2]'
+// CHECK-NEXT: IntegerLiteral 0x{{[0-9a-f]+}} <{{[^:]+}}:56> 'int' 4
+// CHECK-NEXT: IntegerLiteral 0x{{[0-9a-f]+}} <{{[^:]+}}:47> 'int' 1
+// CHECK-NEXT: ImplicitCastExpr
+// CHECK-NEXT: IntegerLiteral 0x{{[0-9a-f]+}} <{{[^:]+}}:50> 'int' 0