[CIR][ABI][NFC] Follow-up to struct unpacking (#791)

This patch fixes a bunch of pending review comments in #784:

 - Remove data layout attribute from address space testing
 - Remove incoherent comment
 - Rename abi_or_pref to abiOrPref
 - Make comments impersonal
 - Implement feature guard for ARM's CMSE secure call feature
 - Track volatile return times feature in CC lowering
 - Track missing features in the Itanium record builder
 - Remove incoherent fix me
 - Clarify comment regarding CIR record layout getter
 - Track missing cache for record layout getter
 - Remove unnecessary todo's

Author: sitio-couto

clang/include/clang/CIR/Dialect/IR/CIRDataLayout.h

@@ -55,7 +55,7 @@ class CIRDataLayout {
   const StructLayout *getStructLayout(mlir::cir::StructType Ty) const;
 
   /// Internal helper method that returns requested alignment for type.
-  llvm::Align getAlignment(mlir::Type Ty, bool abi_or_pref) const;
+  llvm::Align getAlignment(mlir::Type Ty, bool abiOrPref) const;
 
   llvm::Align getABITypeAlign(mlir::Type ty) const {
     return getAlignment(ty, true);
@@ -93,8 +93,6 @@ class CIRDataLayout {
     return layout.getTypeSizeInBits(Ty);
   }
 
-  // The implementation of this method is provided inline as it is particularly
-  // well suited to constant folding when called on a specific Type subclass.
   llvm::TypeSize getTypeSizeInBits(mlir::Type Ty) const;
 
   mlir::Type getIntPtrType(mlir::Type Ty) const {

clang/include/clang/CIR/MissingFeatures.h

@@ -135,6 +135,7 @@ struct MissingFeatures {
   static bool syncScopeID() { return false; }
 
   // Misc
+  static bool cacheRecordLayouts() { return false; }
   static bool capturedByInit() { return false; }
   static bool tryEmitAsConstant() { return false; }
   static bool incrementProfileCounter() { return false; }
@@ -282,6 +283,9 @@ struct MissingFeatures {
   // up argument registers), but we do not yet track such cases.
   static bool chainCall() { return false; }
 
+  // ARM-specific feature that can be specified as a function attribute in C.
+  static bool cmseNonSecureCallAttr() { return false; }
+
   // ABI-lowering has special handling for regcall calling convention (tries to
   // pass every argument in regs). We don't support it just yet.
   static bool regCall() { return false; }
@@ -326,6 +330,9 @@ struct MissingFeatures {
   // CIR modules parsed from text form may not carry the triple or data layout
   // specs. We should make it always present.
   static bool makeTripleAlwaysPresent() { return false; }
+
+  // This Itanium bit is currently being skipped in cir.
+  static bool itaniumRecordLayoutBuilderFinishLayout() { return false; }
 };
 
 } // namespace cir

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

@@ -157,25 +157,25 @@ CIRDataLayout::getStructLayout(mlir::cir::StructType Ty) const {
 }
 
 /*!
-  \param abi_or_pref Flag that determines which alignment is returned. true
+  \param abiOrPref Flag that determines which alignment is returned. true
   returns the ABI alignment, false returns the preferred alignment.
   \param Ty The underlying type for which alignment is determined.
 
-  Get the ABI (\a abi_or_pref == true) or preferred alignment (\a abi_or_pref
+  Get the ABI (\a abiOrPref == true) or preferred alignment (\a abiOrPref
   == false) for the requested type \a Ty.
  */
-llvm::Align CIRDataLayout::getAlignment(mlir::Type Ty, bool abi_or_pref) const {
+llvm::Align CIRDataLayout::getAlignment(mlir::Type Ty, bool abiOrPref) const {
 
   if (llvm::isa<mlir::cir::StructType>(Ty)) {
     // Packed structure types always have an ABI alignment of one.
-    if (::cir::MissingFeatures::recordDeclIsPacked() && abi_or_pref)
+    if (::cir::MissingFeatures::recordDeclIsPacked() && abiOrPref)
       llvm_unreachable("NYI");
 
     // Get the layout annotation... which is lazily created on demand.
     const StructLayout *Layout =
         getStructLayout(llvm::cast<mlir::cir::StructType>(Ty));
     const llvm::Align Align =
-        abi_or_pref ? StructAlignment.ABIAlign : StructAlignment.PrefAlign;
+        abiOrPref ? StructAlignment.ABIAlign : StructAlignment.PrefAlign;
     return std::max(Align, Layout->getAlignment());
   }
 
@@ -184,7 +184,7 @@ llvm::Align CIRDataLayout::getAlignment(mlir::Type Ty, bool abi_or_pref) const {
   assert(!::cir::MissingFeatures::addressSpace());
 
   // Fetch type alignment from MLIR's data layout.
-  unsigned align = abi_or_pref ? layout.getTypeABIAlignment(Ty)
+  unsigned align = abiOrPref ? layout.getTypeABIAlignment(Ty)
                                : layout.getTypePreferredAlignment(Ty);
   return llvm::Align(align);
 }

clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRLowerContext.cpp

@@ -61,8 +61,8 @@ clang::TypeInfo CIRLowerContext::getTypeInfoImpl(const Type T) const {
   // FIXME(cir): Here we fetch the width and alignment of a type considering the
   // current target. We can likely improve this using MLIR's data layout, or
   // some other interface, to abstract this away (e.g. type.getWidth() &
-  // type.getAlign()). I'm not sure if data layoot suffices because this would
-  // involve some other types such as vectors and complex numbers.
+  // type.getAlign()). Verify if data layout suffices because this would involve
+  // some other types such as vectors and complex numbers.
   // FIXME(cir): In the original codegen, this receives an AST type, meaning it
   // differs chars from integers, something that is not possible with the
   // current level of CIR.

clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRRecordLayout.cpp

@@ -12,6 +12,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "CIRRecordLayout.h"
+#include "clang/CIR/MissingFeatures.h"
 
 namespace mlir {
 namespace cir {
@@ -45,10 +46,8 @@ CIRRecordLayout::CIRRecordLayout(
   CXXInfo->NonVirtualAlignment = nonvirtualalignment;
   CXXInfo->PreferredNVAlignment = preferrednvalignment;
   CXXInfo->SizeOfLargestEmptySubobject = SizeOfLargestEmptySubobject;
-  // FIXME(cir): I'm assuming that since we are not dealing with inherited
-  // classes yet, removing the following lines will be ok.
-  // CXXInfo->BaseOffsets = BaseOffsets;
-  // CXXInfo->VBaseOffsets = VBaseOffsets;
+  // FIXME(cir): Initialize base classes offsets.
+  assert(!::cir::MissingFeatures::getCXXRecordBases());
   CXXInfo->HasOwnVFPtr = hasOwnVFPtr;
   CXXInfo->VBPtrOffset = vbptroffset;
   CXXInfo->HasExtendableVFPtr = hasExtendableVFPtr;

clang/lib/CIR/Dialect/Transforms/TargetLowering/LowerCall.cpp

@@ -42,8 +42,8 @@ arrangeFreeFunctionLikeCall(LowerTypes &LT, LowerModule &LM,
   }
 
   // TODO(cir): There's some CC stuff related to no-proto functions here, but
-  // I'm skipping it since it requires CodeGen info. Maybe we can embbed this
-  // information in the FuncOp during CIRGen.
+  // its skipped here since it requires CodeGen info. Maybe this information
+  // could be embbed  in the FuncOp during CIRGen.
 
   assert(!::cir::MissingFeatures::chainCall() && !chainCall && "NYI");
   FnInfoOpts opts = chainCall ? FnInfoOpts::IsChainCall : FnInfoOpts::None;
@@ -120,8 +120,8 @@ void LowerModule::constructAttributeList(StringRef Name,
   // }
 
   // NOTE(cir): The original code adds default and no-builtin attributes here as
-  // well. AFAIK, these are ABI/Target-agnostic, so it would be better handled
-  // in CIRGen. Regardless, I'm leaving this comment here as a heads up.
+  // well. These are ABI/Target-agnostic, so it would be better handled in
+  // CIRGen.
 
   // Override some default IR attributes based on declaration-specific
   // information.

clang/lib/CIR/Dialect/Transforms/TargetLowering/LowerFunction.cpp

@@ -434,8 +434,7 @@ LogicalResult LowerFunction::buildFunctionEpilog(const LowerFunctionInfo &FI) {
 
       // If there is a dominating store to ReturnValue, we can elide
       // the load, zap the store, and usually zap the alloca.
-      // NOTE(cir): This seems like a premature optimization case, so I'm
-      // skipping it.
+      // NOTE(cir): This seems like a premature optimization case. Skipping it.
       if (::cir::MissingFeatures::returnValueDominatingStoreOptmiization()) {
         llvm_unreachable("NYI");
       }
@@ -453,12 +452,6 @@ LogicalResult LowerFunction::buildFunctionEpilog(const LowerFunctionInfo &FI) {
       mlir::PatternRewriter::InsertionGuard guard(rewriter);
       NewFn->walk([&](ReturnOp returnOp) {
         rewriter.setInsertionPoint(returnOp);
-
-        // TODO(cir): I'm not sure if we need this offset here or in CIRGen.
-        // Perhaps both? For now I'm just ignoring it.
-        // Value V = emitAddressAtOffset(*this, getResultAlloca(returnOp),
-        // RetAI);
-
         RV = castReturnValue(returnOp->getOperand(0), RetAI.getCoerceToType(),
                              *this);
         rewriter.replaceOpWithNewOp<ReturnOp>(returnOp, RV);
@@ -655,7 +648,7 @@ Value LowerFunction::rewriteCallOp(const LowerFunctionInfo &CallInfo,
 
   FuncType IRFuncTy = LM.getTypes().getFunctionType(CallInfo);
 
-  // NOTE(cir): Some target/ABI related checks happen here. I'm skipping them
+  // NOTE(cir): Some target/ABI related checks happen here. They are skipped
   // under the assumption that they are handled in CIRGen.
 
   // 1. Set up the arguments.
@@ -737,7 +730,7 @@ Value LowerFunction::rewriteCallOp(const LowerFunctionInfo &CallInfo,
       if (!isa<StructType>(I->getType())) {
         llvm_unreachable("NYI");
       } else {
-        // NOTE(cir): I'm leaving L/RValue stuff for CIRGen to handle.
+        // NOTE(cir): L/RValue stuff are left for CIRGen to handle.
         Src = *I;
       }
 
@@ -756,6 +749,7 @@ Value LowerFunction::rewriteCallOp(const LowerFunctionInfo &CallInfo,
         Value Load = createCoercedValue(Src, ArgInfo.getCoerceToType(), *this);
 
         // FIXME(cir): We should probably handle CMSE non-secure calls here
+        assert(!::cir::MissingFeatures::cmseNonSecureCallAttr());
 
         // since they are a ARM-specific feature.
         if (::cir::MissingFeatures::undef())
@@ -856,22 +850,22 @@ Value LowerFunction::rewriteCallOp(const LowerFunctionInfo &CallInfo,
       // FIXME(cir): Use return value slot here.
       Value RetVal = callOp.getResult();
       // TODO(cir): Check for volatile return values.
+      assert(!::cir::MissingFeatures::volatileTypes());
 
       // NOTE(cir): If the function returns, there should always be a valid
       // return value present. Instead of setting the return value here, we
       // should have the ReturnValueSlot object set it beforehand.
       if (!RetVal) {
         RetVal = callOp.getResult();
         // TODO(cir): Check for volatile return values.
+        assert(::cir::MissingFeatures::volatileTypes());
       }
 
       // An empty record can overlap other data (if declared with
       // no_unique_address); omit the store for such types - as there is no
       // actual data to store.
       if (dyn_cast<StructType>(RetTy) &&
           cast<StructType>(RetTy).getNumElements() != 0) {
-        // NOTE(cir): I'm assuming we don't need to change any offsets here.
-        // Value StorePtr = emitAddressAtOffset(*this, RetVal, RetAI);
         RetVal =
             createCoercedValue(newCallOp.getResult(), RetVal.getType(), *this);
       }

clang/lib/CIR/Dialect/Transforms/TargetLowering/LowerTypes.cpp

@@ -108,8 +108,8 @@ mlir::Type LowerTypes::convertType(Type T) {
   /// LLVM IR representation for a given AST type. When a the ABI-specific
   /// function info sets a nullptr for a return or argument type, the default
   /// type given by this method is used. In CIR's case, its types are already
-  /// supposed to be ABI-specific, so this method is not really useful here. I'm
-  /// keeping it here for parity's sake.
+  /// supposed to be ABI-specific, so this method is not really useful here.
+  /// It's kept here for codegen parity's sake.
 
   // Certain CIR types are already ABI-specific, so we just return them.
   if (isa<BoolType, IntType, SingleType, DoubleType>(T)) {

clang/lib/CIR/Dialect/Transforms/TargetLowering/RecordLayoutBuilder.cpp

@@ -239,18 +239,10 @@ void ItaniumRecordLayoutBuilder::layout(const StructType RT) {
 
   layoutFields(RT);
 
-  // NonVirtualSize = Context.toCharUnitsFromBits(
-  //     llvm::alignTo(getSizeInBits(),
-  //     Context.getTargetInfo().getCharAlign()));
-  // NonVirtualAlignment = Alignment;
-  // PreferredNVAlignment = PreferredAlignment;
-
-  // // Lay out the virtual bases and add the primary virtual base offsets.
-  // LayoutVirtualBases(RD, RD);
-
-  // // Finally, round the size of the total struct up to the alignment
-  // // of the struct itself.
-  // FinishLayout(RD);
+  // FIXME(cir): Handle virtual-related layouts.
+  assert(!::cir::MissingFeatures::getCXXRecordBases());
+
+  assert(!::cir::MissingFeatures::itaniumRecordLayoutBuilderFinishLayout());
 }
 
 void ItaniumRecordLayoutBuilder::initializeLayout(const mlir::Type Ty) {
@@ -558,10 +550,6 @@ static bool mustSkipTailPadding(clang::TargetCXXABI ABI, const StructType RD) {
     return false;
 
   case clang::TargetCXXABI::UseTailPaddingUnlessPOD03:
-    // FIXME: To the extent that this is meant to cover the Itanium ABI
-    // rules, we should implement the restrictions about over-sized
-    // bitfields:
-    //
     // http://itanium-cxx-abi.github.io/cxx-abi/abi.html#POD :
     //   In general, a type is considered a POD for the purposes of
     //   layout if it is a POD type (in the sense of ISO C++
@@ -605,7 +593,8 @@ const CIRRecordLayout &CIRLowerContext::getCIRRecordLayout(const Type D) const {
 
   assert(RT.isComplete() && "Cannot get layout of forward declarations!");
 
-  // FIXME(cir): Cache the layout. Also, use a more MLIR-based approach.
+  // FIXME(cir): Use a more MLIR-based approach by using it's buitin data layout
+  // features, such as interfaces, cacheing, and the DLTI dialect.
 
   const CIRRecordLayout *NewEntry = nullptr;
 
@@ -642,8 +631,8 @@ const CIRRecordLayout &CIRLowerContext::getCIRRecordLayout(const Type D) const {
         Builder.PrimaryBaseIsVirtual, nullptr, false, false);
   }
 
-  // TODO(cir): Cache the layout.
   // TODO(cir): Add option to dump the layouts.
+  assert(!::cir::MissingFeatures::cacheRecordLayouts());
 
   return *NewEntry;
 }

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

@@ -235,9 +235,8 @@ void X86_64ABIInfo::classify(Type Ty, uint64_t OffsetBase, Class &Lo, Class &Hi,
     } else if (isa<IntType>(Ty)) {
 
       // FIXME(cir): Clang's BuiltinType::Kind allow comparisons (GT, LT, etc).
-      // We should implement this in CIR to simplify the conditions below. BTW,
-      // I'm not sure if the comparisons below are truly equivalent to the ones
-      // in Clang.
+      // We should implement this in CIR to simplify the conditions below. Hence,
+      // Comparisons below might not be truly equivalent to the ones in Clang.
       if (isa<IntType>(Ty)) {
         Current = Class::Integer;
       }

clang/test/CIR/Lowering/address-space.cir

@@ -5,8 +5,7 @@
 
 module attributes {
   cir.triple = "spirv64-unknown-unknown",
-  llvm.data_layout = "e-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-G1",
-  dlti.dl_spec = #dlti.dl_spec<> // Avoid assert errors.
+  llvm.data_layout = "e-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-G1"
 } {
   cir.global external addrspace(offload_global) @addrspace1 = #cir.int<1> : !s32i
   // LLVM: @addrspace1 = addrspace(1) global i32