Address review feedback

Author: andykaylor

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

@@ -2620,31 +2620,25 @@ def CIR_VTableAddrPointOp : CIR_Op<"vtable.address_point", [
   }];
 
   let arguments = (ins
-    OptionalAttr<FlatSymbolRefAttr>:$name,
-    Optional<CIR_AnyType>:$sym_addr,
+    FlatSymbolRefAttr:$name,
     CIR_AddressPointAttr:$address_point
   );
 
   let results = (outs Res<CIR_PtrToVPtr, "", []>:$addr);
 
   let assemblyFormat = [{
     `(`
-      ($name^)?
-      ($sym_addr^ `:` type($sym_addr))?
-      `,`
-        `address_point` `=` $address_point
+      $name `,` `address_point` `=` $address_point
     `)`
     `:` qualified(type($addr)) attr-dict
   }];
-
-  let hasVerifier = 1;
 }
 
 //===----------------------------------------------------------------------===//
-// VTableGetVptr
+// VTableGetVPtr
 //===----------------------------------------------------------------------===//
 
-def CIR_VTableGetVptrOp : CIR_Op<"vtable.get_vptr", [Pure]> {
+def CIR_VTableGetVPtrOp : CIR_Op<"vtable.get_vptr", [Pure]> {
   let summary = "Get a the address of the vtable pointer for an object";
   let description = [{
     The `vtable.get_vptr` operation retrieves the address of the vptr for a
@@ -2663,13 +2657,13 @@ def CIR_VTableGetVptrOp : CIR_Op<"vtable.get_vptr", [Pure]> {
   }];
 
   let arguments = (ins
-                   Arg<CIR_PointerType, "the vptr address", [MemRead]>:$src);
-
-  let results = (outs CIR_PtrToVPtr:$vptr_ty);
+    Arg<CIR_PointerType, "the vptr address", [MemRead]>:$src
+  );
 
+  let results = (outs CIR_PtrToVPtr:$result);
 
   let assemblyFormat = [{
-      $src `:` qualified(type($src)) `->` qualified(type($vptr_ty)) attr-dict
+      $src `:` qualified(type($src)) `->` qualified(type($result)) attr-dict
   }];
 
 }
@@ -2689,14 +2683,18 @@ def CIR_VTableGetVirtualFnAddrOp : CIR_Op<"vtable.get_virtual_fn_addr", [
     the address of the virtual function pointer, which can then be loaded and
     called.
 
+    The `vptr` operand must be a `!cir.ptr<!cir.vptr>` value, which would
+    have been returned by a previous call to `cir.vatble.get_vptr`. The
+    `index` operand is an index of the virtual function in the vtable.
+
     The return type is a pointer-to-pointer to the function type.
 
     Example:
     ```mlir
     %2 = cir.load %0 : !cir.ptr<!cir.ptr<!rec_C>>, !cir.ptr<!rec_C>
     %3 = cir.vtable.get_vptr %2 : !cir.ptr<!rec_C> -> !cir.ptr<!cir.vptr>
     %4 = cir.load %3 : !cir.ptr<!cir.vptr>, !cir.vptr
-    %5 = cir.vtable.get_virtual_fn_addr(%4, index = 2) : !cir.vptr
+    %5 = cir.vtable.get_virtual_fn_addr %4[2] : !cir.vptr
                   -> !cir.ptr<!cir.ptr<!cir.func<(!cir.ptr<!rec_C>) -> !s32i>>>
     %6 = cir.load align(8) %5 : !cir.ptr<!cir.ptr<!cir.func<(!cir.ptr<!rec_C>)
                                                                  -> !s32i>>>,
@@ -2708,30 +2706,13 @@ def CIR_VTableGetVirtualFnAddrOp : CIR_Op<"vtable.get_virtual_fn_addr", [
 
   let arguments = (ins
     Arg<CIR_VPtrType, "vptr", [MemRead]>:$vptr,
-    IndexAttr:$index_attr);
+    I64Attr:$index);
 
-  let results = (outs CIR_PointerType:$vfptr_ty);
+  let results = (outs CIR_PointerType:$result);
 
   let assemblyFormat = [{
-    `(`
-      $vptr `,` `index` `=` $index_attr
-    `)`
-    `:` qualified(type($vptr)) `,` qualified(type($vfptr_ty)) attr-dict
-  }];
-
-  let builders = [
-    OpBuilder<(ins "mlir::Type":$type,
-                   "mlir::Value":$value,
-                   "unsigned":$index),
-    [{
-      mlir::APInt fnIdx(64, index);
-      build($_builder, $_state, type, value, fnIdx);
-    }]>
-  ];
-
-  let extraClassDeclaration = [{
-    /// Return the index of the record member being accessed.
-    uint64_t getIndex() { return getIndexAttr().getZExtValue(); }
+    $vptr `[` $index `]` attr-dict
+    `:` qualified(type($vptr)) `->` qualified(type($result))
   }];
 }
 

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

@@ -267,8 +267,7 @@ def CIR_AnyDataMemberType : CIR_TypeBase<"::cir::DataMemberType",
 // VPtr type predicates
 //===----------------------------------------------------------------------===//
 
-def CIR_AnyVPtrType : CIR_TypeBase<"::cir::VPtrType",
-    "vptr type">;
+def CIR_AnyVPtrType : CIR_TypeBase<"::cir::VPtrType", "vptr type">;
 
 def CIR_PtrToVPtr : CIR_PtrToType<CIR_AnyVPtrType>;
 

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

@@ -347,8 +347,9 @@ def CIR_DataMemberType : CIR_Type<"DataMember", "data_member",
 // CIR_VPtrType
 //===----------------------------------------------------------------------===//
 
-def CIR_VPtrType : CIR_Type<"VPtr", "vptr",
-    [DeclareTypeInterfaceMethods<DataLayoutTypeInterface>]> {
+def CIR_VPtrType : CIR_Type<"VPtr", "vptr", [
+  DeclareTypeInterfaceMethods<DataLayoutTypeInterface>
+]> {
 
   let summary = "CIR type that is used for the vptr member of C++ objects";
   let description = [{

clang/lib/CIR/CodeGen/CIRGenClass.cpp

@@ -1705,7 +1705,7 @@ void CIRGenFunction::emitTypeMetadataCodeForVCall(const CXXRecordDecl *RD,
 
 mlir::Value CIRGenFunction::getVTablePtr(mlir::Location Loc, Address This,
                                          const CXXRecordDecl *RD) {
-  auto VTablePtr = builder.create<cir::VTableGetVptrOp>(
+  auto VTablePtr = builder.create<cir::VTableGetVPtrOp>(
       Loc, builder.getPtrToVPtrType(), This.getPointer());
   Address VTablePtrAddr = Address(VTablePtr, This.getAlignment());
 

clang/lib/CIR/CodeGen/CIRGenItaniumCXXABI.cpp

@@ -935,8 +935,9 @@ cir::GlobalOp CIRGenItaniumCXXABI::getAddrOfVTable(const CXXRecordDecl *RD,
 CIRGenCallee CIRGenItaniumCXXABI::getVirtualFunctionPointer(
     CIRGenFunction &CGF, GlobalDecl GD, Address This, mlir::Type Ty,
     SourceLocation Loc) {
+  auto &builder = CGM.getBuilder();
   auto loc = CGF.getLoc(Loc);
-  auto TyPtr = CGF.getBuilder().getPointerTo(Ty);
+  auto TyPtr = builder.getPointerTo(Ty);
   auto *MethodDecl = cast<CXXMethodDecl>(GD.getDecl());
   auto VTable = CGF.getVTablePtr(loc, This, MethodDecl->getParent());
 
@@ -951,11 +952,10 @@ CIRGenCallee CIRGenItaniumCXXABI::getVirtualFunctionPointer(
     if (CGM.getItaniumVTableContext().isRelativeLayout()) {
       llvm_unreachable("NYI");
     } else {
-      auto VTableSlotPtr =
-          CGF.getBuilder().create<cir::VTableGetVirtualFnAddrOp>(
-              loc, CGF.getBuilder().getPointerTo(TyPtr), VTable, VTableIndex);
-      VFuncLoad = CGF.getBuilder().createAlignedLoad(loc, TyPtr, VTableSlotPtr,
-                                                     CGF.getPointerAlign());
+      auto VTableSlotPtr = builder.create<cir::VTableGetVirtualFnAddrOp>(
+          loc, builder.getPointerTo(TyPtr), VTable, VTableIndex);
+      VFuncLoad = builder.createAlignedLoad(loc, TyPtr, VTableSlotPtr,
+                                            CGF.getPointerAlign());
     }
 
     // Add !invariant.load md to virtual function load to indicate that
@@ -1013,7 +1013,7 @@ CIRGenItaniumCXXABI::getVTableAddressPoint(BaseSubobject Base,
 
   return builder.create<cir::VTableAddrPointOp>(
       CGM.getLoc(VTableClass->getSourceRange()), vtablePtrTy,
-      mlir::FlatSymbolRefAttr::get(vtable.getSymNameAttr()), mlir::Value{},
+      mlir::FlatSymbolRefAttr::get(vtable.getSymNameAttr()),
       cir::AddressPointAttr::get(CGM.getBuilder().getContext(),
                                  AddressPoint.VTableIndex,
                                  AddressPoint.AddressPointIndex));

clang/lib/CIR/Dialect/IR/CIRDialect.cpp

@@ -2396,10 +2396,7 @@ cir::GetGlobalOp::verifySymbolUses(SymbolTableCollection &symbolTable) {
 
 LogicalResult
 cir::VTableAddrPointOp::verifySymbolUses(SymbolTableCollection &symbolTable) {
-  // vtable ptr is not coming from a symbol.
-  if (!getName())
-    return success();
-  auto name = *getName();
+  StringRef name = getName();
 
   // Verify that the result type underlying pointer type matches the type of
   // the referenced cir.global or cir.func op.
@@ -2417,24 +2414,6 @@ cir::VTableAddrPointOp::verifySymbolUses(SymbolTableCollection &symbolTable) {
   return success();
 }
 
-LogicalResult cir::VTableAddrPointOp::verify() {
-  // The operation uses either a symbol or a value to operate, but not both
-  if (getName() && getSymAddr())
-    return emitOpError("should use either a symbol or value, but not both");
-
-  // If not a symbol, stick with the concrete type used for getSymAddr.
-  if (getSymAddr())
-    return success();
-
-  auto resultType = getAddr().getType();
-  auto resTy = cir::PointerType::get(cir::VPtrType::get(getContext()));
-
-  if (resultType != resTy)
-    return emitOpError("result type must be '")
-           << resTy << "', but provided result type is '" << resultType << "'";
-  return success();
-}
-
 //===----------------------------------------------------------------------===//
 // VTTAddrPointOp
 //===----------------------------------------------------------------------===//

clang/lib/CIR/Dialect/Transforms/TargetLowering/Targets/LoweringPrepareItaniumCXXABI.cpp

@@ -118,7 +118,7 @@ buildDynamicCastToVoidAfterNullCheck(CIRBaseBuilderTy &builder,
   auto vptrTy = cir::VPtrType::get(builder.getContext());
   auto vptrPtrTy = builder.getPointerTo(vptrTy);
   auto vptrPtr =
-      builder.create<cir::VTableGetVptrOp>(loc, vptrPtrTy, op.getSrc());
+      builder.create<cir::VTableGetVPtrOp>(loc, vptrPtrTy, op.getSrc());
   auto vptr = builder.createLoad(loc, vptrPtr);
   auto elementPtr =
       builder.createBitcast(vptr, builder.getPointerTo(vtableElemTy));

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

@@ -3866,22 +3866,13 @@ mlir::LogicalResult CIRToLLVMVTableAddrPointOpLowering::matchAndRewrite(
     mlir::ConversionPatternRewriter &rewriter) const {
   const auto *converter = getTypeConverter();
   auto targetType = converter->convertType(op.getType());
-  mlir::Value symAddr = op.getSymAddr();
   llvm::SmallVector<mlir::LLVM::GEPArg> offsets;
   mlir::Type eltType;
-  if (!symAddr) {
-    symAddr = getValueForVTableSymbol(op, rewriter, getTypeConverter(),
-                                      op.getNameAttr(), eltType);
-    offsets = llvm::SmallVector<mlir::LLVM::GEPArg>{
-        0, op.getAddressPointAttr().getIndex(),
-        op.getAddressPointAttr().getOffset()};
-  } else {
-    // Get indirect vtable address point retrieval
-    symAddr = adaptor.getSymAddr();
-    eltType = converter->convertType(symAddr.getType());
-    offsets = llvm::SmallVector<mlir::LLVM::GEPArg>{
-        op.getAddressPointAttr().getOffset()};
-  }
+  mlir::Value symAddr = getValueForVTableSymbol(
+      op, rewriter, getTypeConverter(), op.getNameAttr(), eltType);
+  offsets = llvm::SmallVector<mlir::LLVM::GEPArg>{
+      0, op.getAddressPointAttr().getIndex(),
+      op.getAddressPointAttr().getOffset()};
 
   assert(eltType && "Shouldn't ever be missing an eltType here");
   rewriter.replaceOpWithNewOp<mlir::LLVM::GEPOp>(op, targetType, eltType,
@@ -3890,8 +3881,8 @@ mlir::LogicalResult CIRToLLVMVTableAddrPointOpLowering::matchAndRewrite(
   return mlir::success();
 }
 
-mlir::LogicalResult CIRToLLVMVTableGetVptrOpLowering::matchAndRewrite(
-    cir::VTableGetVptrOp op, OpAdaptor adaptor,
+mlir::LogicalResult CIRToLLVMVTableGetVPtrOpLowering::matchAndRewrite(
+    cir::VTableGetVPtrOp op, OpAdaptor adaptor,
     mlir::ConversionPatternRewriter &rewriter) const {
   // cir.vtable.get_vptr is equivalent to a bitcast from the source object
   // pointer to the vptr type. Since the LLVM dialect uses opaque pointers
@@ -4565,7 +4556,7 @@ void populateCIRToLLVMConversionPatterns(
       CIRToLLVMVecSplatOpLowering,
       CIRToLLVMVecTernaryOpLowering,
       CIRToLLVMVTableAddrPointOpLowering,
-      CIRToLLVMVTableGetVptrOpLowering,
+      CIRToLLVMVTableGetVPtrOpLowering,
       CIRToLLVMVTableGetVirtualFnAddrOpLowering,
       CIRToLLVMVTTAddrPointOpLowering
 #define GET_BUILTIN_LOWERING_LIST

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

@@ -1034,13 +1034,13 @@ class CIRToLLVMVTableAddrPointOpLowering
                   mlir::ConversionPatternRewriter &) const override;
 };
 
-class CIRToLLVMVTableGetVptrOpLowering
-    : public mlir::OpConversionPattern<cir::VTableGetVptrOp> {
+class CIRToLLVMVTableGetVPtrOpLowering
+    : public mlir::OpConversionPattern<cir::VTableGetVPtrOp> {
 public:
-  using mlir::OpConversionPattern<cir::VTableGetVptrOp>::OpConversionPattern;
+  using mlir::OpConversionPattern<cir::VTableGetVPtrOp>::OpConversionPattern;
 
   mlir::LogicalResult
-  matchAndRewrite(cir::VTableGetVptrOp op, OpAdaptor,
+  matchAndRewrite(cir::VTableGetVPtrOp op, OpAdaptor,
                   mlir::ConversionPatternRewriter &) const override;
 };
 

clang/test/CIR/CodeGen/derived-to-base.cpp

@@ -120,7 +120,7 @@ void vcall(C1 &c1) {
 // CHECK:   %6 = cir.load{{.*}} %3 : !cir.ptr<!rec_buffy>, !rec_buffy
 // CHECK:   %7 = cir.vtable.get_vptr %4 : !cir.ptr<!rec_C1> -> !cir.ptr<!cir.vptr>
 // CHECK:   %8 = cir.load{{.*}} %7 : !cir.ptr<!cir.vptr>, !cir.vptr
-// CHECK:   %9 = cir.vtable.get_virtual_fn_addr(%8, index = 2) : !cir.vptr, !cir.ptr<!cir.ptr<!cir.func<(!cir.ptr<!rec_C1>, !s32i, !rec_buffy) -> !s32i>>>
+// CHECK:   %9 = cir.vtable.get_virtual_fn_addr %8[2] : !cir.vptr -> !cir.ptr<!cir.ptr<!cir.func<(!cir.ptr<!rec_C1>, !s32i, !rec_buffy) -> !s32i>>>
 // CHECK:   %10 = cir.load align(8) %9 : !cir.ptr<!cir.ptr<!cir.func<(!cir.ptr<!rec_C1>, !s32i, !rec_buffy) -> !s32i>>>, !cir.ptr<!cir.func<(!cir.ptr<!rec_C1>, !s32i, !rec_buffy) -> !s32i>>
 // CHECK:   %11 = cir.call %10(%4, %5, %6) : (!cir.ptr<!cir.func<(!cir.ptr<!rec_C1>, !s32i, !rec_buffy) -> !s32i>>, !cir.ptr<!rec_C1>, !s32i, !rec_buffy) -> !s32i
 // CHECK:   cir.return