Address review feedback

Author: mmha

clang/include/clang/CIR/Dialect/IR/CIROps.td

@@ -3696,10 +3696,10 @@ def CIR_VAEndOp : CIR_Op<"va_end"> {
   }];
 }
 
-def CIR_VAArgOp : CIR_Op<"va.arg"> {
+def CIR_VAArgOp : CIR_Op<"va_arg"> {
   let summary = "Fetches next variadic element as a given type";
   let description = [{
-    The `cir.va.arg` operation models the C/C++ `va_arg` macro by reading the
+    The `cir.va_arg` operation models the C/C++ `va_arg` macro by reading the
     next argument from an active variable argument list and producing it as a
     value of a specified result type.
 
@@ -3708,16 +3708,12 @@ def CIR_VAArgOp : CIR_Op<"va.arg"> {
     the fetched value as the result, whose type is chosen by the user of the
     operation.
 
-    A `cir.va.arg` must only be used on a `va_list` that has been initialized
+    A `cir.va_arg` must only be used on a `va_list` that has been initialized
     with `cir.va.start` and not yet finalized by `cir.va.end`. The semantics
     (including alignment and promotion rules) follow the platform ABI; the
     frontend is responsible for providing a `va_list` pointer that matches the
     target representation.
 
-    Unless replaced by LoweringPrepare for the chosen target ABI this maps to
-    LLVM's `va_arg` instruction, yielding a value of the requested result type
-    and updating the underlying `va_list` pointer.
-
     Example:
     ```mlir
     // %args : !cir.ptr<!cir.array<!rec___va_list_tag x 1>>
@@ -3727,7 +3723,7 @@ def CIR_VAArgOp : CIR_Op<"va.arg"> {
     cir.va.start %p : !cir.ptr<!rec___va_list_tag>
 
     // Fetch an `int` from the vararg list.
-    %v = cir.va.arg %p : (!cir.ptr<!rec___va_list_tag>) -> !s32i
+    %v = cir.va_arg %p : (!cir.ptr<!rec___va_list_tag>) -> !s32i
 
     cir.va.end %p : !cir.ptr<!rec___va_list_tag>
     ```

clang/include/clang/CIR/MissingFeatures.h

@@ -279,6 +279,7 @@ struct MissingFeatures {
   static bool vtableInitialization() { return false; }
   static bool vtableRelativeLayout() { return false; }
   static bool msvcBuiltins() { return false; }
+  static bool vaArgABILowering() { return false; }
   static bool vlas() { return false; }
 
   // Missing types

clang/lib/CIR/CodeGen/CIRGenBuiltin.cpp

@@ -402,10 +402,12 @@ void CIRGenFunction::emitVAEnd(mlir::Value vaList) {
   cir::VAEndOp::create(builder, vaList.getLoc(), vaList);
 }
 
-// FIXME(cir): This completely abstracts away the ABI with a generic CIR Op. We
-// need to decide how to handle va_arg target-specific codegen.
-mlir::Value CIRGenFunction::emitVAArg(VAArgExpr *ve, Address &vaListAddr) {
+// FIXME(cir): This completely abstracts away the ABI with a generic CIR Op. By
+// default this lowers to llvm.va_arg which is incomplete and not ABI-compliant
+// on most targets so cir.va_arg will need some ABI handling in LoweringPrepare
+mlir::Value CIRGenFunction::emitVAArg(VAArgExpr *ve) {
   assert(!cir::MissingFeatures::msabi());
+  assert(!cir::MissingFeatures::vlas());
   mlir::Location loc = cgm.getLoc(ve->getExprLoc());
   mlir::Type type = convertType(ve->getType());
   mlir::Value vaList = emitVAListRef(ve->getSubExpr()).getPointer();

clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp

@@ -400,16 +400,14 @@ class ScalarExprEmitter : public StmtVisitor<ScalarExprEmitter, mlir::Value> {
   }
 
   mlir::Value VisitVAArgExpr(VAArgExpr *ve) {
-    QualType Ty = ve->getType();
+    QualType ty = ve->getType();
 
-    if (Ty->isVariablyModifiedType()) {
-      cgf.cgm.errorNYI(ve->getSourceRange(), "variably modified types in varargs");
+    if (ty->isVariablyModifiedType()) {
+      cgf.cgm.errorNYI(ve->getSourceRange(),
+                       "variably modified types in varargs");
     }
 
-    Address argValue = Address::invalid();
-    mlir::Value val = cgf.emitVAArg(ve, argValue);
-
-    return val;
+    return cgf.emitVAArg(ve);
   }
 
   mlir::Value VisitUnaryExprOrTypeTraitExpr(const UnaryExprOrTypeTraitExpr *e);

clang/lib/CIR/CodeGen/CIRGenFunction.h

@@ -1513,7 +1513,7 @@ class CIRGenFunction : public CIRGenTypeCache {
   /// either \c emitVAListRef or \c emitMSVAListRef.
   ///
   /// \returns SSA value with the argument.
-  mlir::Value emitVAArg(VAArgExpr *ve, Address &vaListAddr);
+  mlir::Value emitVAArg(VAArgExpr *ve);
 
   /// ----------------------
   /// CIR build helpers

clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp

@@ -3152,6 +3152,7 @@ mlir::LogicalResult CIRToLLVMVAEndOpLowering::matchAndRewrite(
 mlir::LogicalResult CIRToLLVMVAArgOpLowering::matchAndRewrite(
     cir::VAArgOp op, OpAdaptor adaptor,
     mlir::ConversionPatternRewriter &rewriter) const {
+  assert(!cir::MissingFeatures::vaArgABILowering());
   auto opaquePtr = mlir::LLVM::LLVMPointerType::get(getContext());
   auto vaList = mlir::LLVM::BitcastOp::create(rewriter, op.getLoc(), opaquePtr,
                                               adaptor.getArgList());

clang/test/CIR/CodeGen/var_arg.c

@@ -27,7 +27,7 @@ int varargs(int count, ...) {
 // CIR:   %[[COUNT_VAL:.+]] = cir.load{{.*}} %[[COUNT_ADDR]] : !cir.ptr<!s32i>, !s32i
 // CIR:   cir.va_start %[[VA_PTR0]] %[[COUNT_VAL]] : !cir.ptr<!rec___va_list_tag>, !s32i
 // CIR:   %[[VA_PTR1:.+]] = cir.cast(array_to_ptrdecay, %[[VAAREA]] : !cir.ptr<!cir.array<!rec___va_list_tag x 1>>), !cir.ptr<!rec___va_list_tag>
-// CIR:   %[[VA_ARG:.+]] = cir.va.arg %[[VA_PTR1]] : (!cir.ptr<!rec___va_list_tag>) -> !s32i
+// CIR:   %[[VA_ARG:.+]] = cir.va_arg %[[VA_PTR1]] : (!cir.ptr<!rec___va_list_tag>) -> !s32i
 // CIR:   cir.store{{.*}} %[[VA_ARG]], %[[RES_ADDR]] : !s32i, !cir.ptr<!s32i>
 // CIR:   %[[VA_PTR2:.+]] = cir.cast(array_to_ptrdecay, %[[VAAREA]] : !cir.ptr<!cir.array<!rec___va_list_tag x 1>>), !cir.ptr<!rec___va_list_tag>
 // CIR:   cir.va_end %[[VA_PTR2]] : !cir.ptr<!rec___va_list_tag>
@@ -103,7 +103,7 @@ int stdarg_start(int count, ...) {
 // CIR:   %[[C12345:.+]] = cir.const #cir.int<12345> : !s32i
 // CIR:   cir.va_start %[[VA_PTR0]] %[[C12345]] : !cir.ptr<!rec___va_list_tag>, !s32i
 // CIR:   %[[VA_PTR1:.+]] = cir.cast(array_to_ptrdecay, %[[VAAREA]] : !cir.ptr<!cir.array<!rec___va_list_tag x 1>>), !cir.ptr<!rec___va_list_tag>
-// CIR:   %[[VA_ARG:.+]] = cir.va.arg %[[VA_PTR1]] : (!cir.ptr<!rec___va_list_tag>) -> !s32i
+// CIR:   %[[VA_ARG:.+]] = cir.va_arg %[[VA_PTR1]] : (!cir.ptr<!rec___va_list_tag>) -> !s32i
 // CIR:   cir.store{{.*}} %[[VA_ARG]], %[[RES_ADDR]] : !s32i, !cir.ptr<!s32i>
 // CIR:   %[[VA_PTR2:.+]] = cir.cast(array_to_ptrdecay, %[[VAAREA]] : !cir.ptr<!cir.array<!rec___va_list_tag x 1>>), !cir.ptr<!rec___va_list_tag>
 // CIR:   cir.va_end %[[VA_PTR2]] : !cir.ptr<!rec___va_list_tag>