[CIR][ABI] Implement basic struct CC lowering for x86_64 #784

sitio-couto · 2024-08-11T13:39:51Z

This patch adds the necessary bits for unraveling struct arguments and
return values for the x86_64 calling convention.

…ctor

This patch adds the necessary bits for unraveling struct arguments and return values for the x86_64 calling convention.

bcardosolopes

I'm going to approve and land, please address all comments in follow up PRs.

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

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

bcardosolopes · 2024-08-12T23:22:09Z

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

@@ -458,6 +458,10 @@ LogicalResult CastOp::verify() {
    return success();
  }
  case cir::CastKind::bitcast: {
+    // Allow bitcast of structs for calling conventions.
+    if (isa<StructType>(srcType) || isa<StructType>(resType))


Feels hacky, I don't see documentation supporting this or a good explanation of the semantics. Sounds like we need a new style of cast operation instead.

Feels hacky, I don't see documentation supporting this or a good explanation of the semantics.

I don't think I follow. I'ts a bitcast. We reinterpret struct bits as integers for the calling convention.

Sounds like we need a new style of cast operation instead.

Do you have something in mind?

bcardosolopes · 2024-08-12T23:23:08Z

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

@@ -42,7 +43,7 @@ class CIRLowerContext : public llvm::RefCountedBase<CIRLowerContext> {

  /// The language options used to create the AST associated with
  /// this ASTContext object.
-  clang::LangOptions &LangOpts;
+  clang::LangOptions LangOpts;


Why do we now need to make a new copy of this? I'd expect langoptions to survive til the end.

Someone has to retain ownership of LangOpts to prevent dangling refs. CIRLowerContext is the owner in this case. I can try using move semantics here instead.

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

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

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

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

bcardosolopes · 2024-08-12T23:41:21Z

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

+
+// FIXME(cir): Create a custom rewriter class to abstract this away.
+Value createBitcast(Value Src, Type Ty, LowerFunction &LF) {
+  return LF.getRewriter().create<CastOp>(Src.getLoc(), Ty, CastKind::bitcast,


Is this where the bitcast between struct comes from?

In the literal sense, yes, this line creates it. Semantically, we need the bitcast to circumvent MLIR's type-checking and properly abide by the calling convention by re-interpreting structs as integers.

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

This patch fixes a bunch of pending review comments: - 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 Itanium record builder - Remove incoherent fixme - Clarify comment regarding CIR record layout getter - Track missing cache for record layout getter - Remove unecessary todo's

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

This patch adds the necessary bits for unraveling struct arguments and return values for the x86_64 calling convention.

This patch fixes a bunch of pending review comments in llvm#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