Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Clang] NFC: Move Arm type attributes to separate trailing object. #78424

Merged
merged 2 commits into from
Jan 18, 2024

Conversation

sdesmalen-arm
Copy link
Collaborator

This decouples the Arm type attributes from other bits, which means
the data will only be allocated when a function uses these Arm
attributes.

The first patch adds the bit HasArmTypeAttributes to
FunctionTypeBitfields, which grows from 62 bits to 63 bits.

In the second patch, I've moved this bit (HasArmTypeAttributes) to
FunctionTypeExtraBitfields, because it looks like the bits in
FunctionTypeBitfields are precious and we really don't want that struct
to grow beyond 64 bits.

I've split this out into two patches to explain the rationale, but those
can be squashed before merging.

This decouples the Arm type attributes from other bits, which means
the data will only be allocated when a function uses these Arm
attributes.

The first patch adds the bit `HasArmTypeAttributes` to
`FunctionTypeBitfields`, which grows from 62 bits to 63 bits.

In the second patch, I've moved this bit (`HasArmTypeAttributes`) to
`FunctionTypeExtraBitfields`, because it looks like the bits in
`FunctionTypeBitfields` are precious and we really don't want that struct
to grow beyond 64 bits.

I've split this out into two patches to explain the rationale, but those
can be squashed before merging.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 17, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 17, 2024

@llvm/pr-subscribers-clang

Author: Sander de Smalen (sdesmalen-arm)

Changes

This decouples the Arm type attributes from other bits, which means
the data will only be allocated when a function uses these Arm
attributes.

The first patch adds the bit HasArmTypeAttributes to
FunctionTypeBitfields, which grows from 62 bits to 63 bits.

In the second patch, I've moved this bit (HasArmTypeAttributes) to
FunctionTypeExtraBitfields, because it looks like the bits in
FunctionTypeBitfields are precious and we really don't want that struct
to grow beyond 64 bits.

I've split this out into two patches to explain the rationale, but those
can be squashed before merging.


Full diff: https://github.com/llvm/llvm-project/pull/78424.diff

3 Files Affected:

  • (modified) clang/include/clang/AST/Type.h (+43-16)
  • (modified) clang/lib/AST/ASTContext.cpp (+4-3)
  • (modified) clang/lib/AST/Type.cpp (+10-2)
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index a7efe78591635e..259e920acf9ff3 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -4029,6 +4029,22 @@ class FunctionType : public Type {
   /// because TrailingObjects cannot handle repeated types.
   struct ExceptionType { QualType Type; };
 
+  /// A simple holder for various uncommon bits which do not fit in
+  /// FunctionTypeBitfields. Aligned to alignof(void *) to maintain the
+  /// alignment of subsequent objects in TrailingObjects.
+  struct alignas(void *) FunctionTypeExtraBitfields {
+    /// The number of types in the exception specification.
+    /// A whole unsigned is not needed here and according to
+    /// [implimits] 8 bits would be enough here.
+    unsigned NumExceptionType : 10;
+
+    LLVM_PREFERRED_TYPE(bool)
+    unsigned HasArmTypeAttributes : 1;
+
+    FunctionTypeExtraBitfields()
+        : NumExceptionType(0), HasArmTypeAttributes(false) {}
+  };
+
   /// The AArch64 SME ACLE (Arm C/C++ Language Extensions) define a number
   /// of function type attributes that can be set on function types, including
   /// function pointers.
@@ -4042,7 +4058,8 @@ class FunctionType : public Type {
     SME_ZAMask = 0b111 << SME_ZAShift,
 
     SME_AttributeMask = 0b111'111 // We only support maximum 6 bits because of
-                                  // the bitmask in FunctionTypeExtraBitfields.
+                                  // the bitmask in FunctionTypeArmAttributes
+                                  // and ExtProtoInfo.
   };
 
   enum ArmStateValue : unsigned {
@@ -4057,20 +4074,15 @@ class FunctionType : public Type {
     return (ArmStateValue)((AttrBits & SME_ZAMask) >> SME_ZAShift);
   }
 
-  /// A simple holder for various uncommon bits which do not fit in
-  /// FunctionTypeBitfields. Aligned to alignof(void *) to maintain the
-  /// alignment of subsequent objects in TrailingObjects.
-  struct alignas(void *) FunctionTypeExtraBitfields {
-    /// The number of types in the exception specification.
-    /// A whole unsigned is not needed here and according to
-    /// [implimits] 8 bits would be enough here.
-    unsigned NumExceptionType : 10;
-
+  /// A holder for Arm type attributes as described in the Arm C/C++
+  /// Language extensions which are not particularly common to all
+  /// types and therefore accounted separately from FunctionTypeBitfields.
+  struct alignas(void *) FunctionTypeArmAttributes {
     /// Any AArch64 SME ACLE type attributes that need to be propagated
     /// on declarations and function pointers.
     unsigned AArch64SMEAttributes : 6;
-    FunctionTypeExtraBitfields()
-        : NumExceptionType(0), AArch64SMEAttributes(SME_NormalFunction) {}
+
+    FunctionTypeArmAttributes() : AArch64SMEAttributes(SME_NormalFunction) {}
   };
 
 protected:
@@ -4169,7 +4181,8 @@ class FunctionProtoType final
       public llvm::FoldingSetNode,
       private llvm::TrailingObjects<
           FunctionProtoType, QualType, SourceLocation,
-          FunctionType::FunctionTypeExtraBitfields, FunctionType::ExceptionType,
+          FunctionType::FunctionTypeExtraBitfields,
+          FunctionType::FunctionTypeArmAttributes, FunctionType::ExceptionType,
           Expr *, FunctionDecl *, FunctionType::ExtParameterInfo, Qualifiers> {
   friend class ASTContext; // ASTContext creates these.
   friend TrailingObjects;
@@ -4276,7 +4289,11 @@ class FunctionProtoType final
 
     bool requiresFunctionProtoTypeExtraBitfields() const {
       return ExceptionSpec.Type == EST_Dynamic ||
-             AArch64SMEAttributes != SME_NormalFunction;
+             requiresFunctionProtoTypeArmAttributes();
+    }
+
+    bool requiresFunctionProtoTypeArmAttributes() const {
+      return AArch64SMEAttributes != SME_NormalFunction;
     }
 
     void setArmSMEAttribute(AArch64SMETypeAttributes Kind, bool Enable = true) {
@@ -4296,6 +4313,10 @@ class FunctionProtoType final
     return isVariadic();
   }
 
+  unsigned numTrailingObjects(OverloadToken<FunctionTypeArmAttributes>) const {
+    return hasArmTypeAttributes();
+  }
+
   unsigned numTrailingObjects(OverloadToken<FunctionTypeExtraBitfields>) const {
     return hasExtraBitfields();
   }
@@ -4384,6 +4405,12 @@ class FunctionProtoType final
 
   }
 
+  bool hasArmTypeAttributes() const {
+    return FunctionTypeBits.HasExtraBitfields &&
+           getTrailingObjects<FunctionTypeExtraBitfields>()
+               ->HasArmTypeAttributes;
+  }
+
   bool hasExtQualifiers() const {
     return FunctionTypeBits.HasExtQuals;
   }
@@ -4595,9 +4622,9 @@ class FunctionProtoType final
   /// Return a bitmask describing the SME attributes on the function type, see
   /// AArch64SMETypeAttributes for their values.
   unsigned getAArch64SMEAttributes() const {
-    if (!hasExtraBitfields())
+    if (!hasArmTypeAttributes())
       return SME_NormalFunction;
-    return getTrailingObjects<FunctionTypeExtraBitfields>()
+    return getTrailingObjects<FunctionTypeArmAttributes>()
         ->AArch64SMEAttributes;
   }
 
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index d9cefcaa84d7e5..0fc0831b221aab 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -4484,10 +4484,11 @@ QualType ASTContext::getFunctionTypeInternal(
       EPI.ExceptionSpec.Type, EPI.ExceptionSpec.Exceptions.size());
   size_t Size = FunctionProtoType::totalSizeToAlloc<
       QualType, SourceLocation, FunctionType::FunctionTypeExtraBitfields,
-      FunctionType::ExceptionType, Expr *, FunctionDecl *,
-      FunctionProtoType::ExtParameterInfo, Qualifiers>(
+      FunctionType::FunctionTypeArmAttributes, FunctionType::ExceptionType,
+      Expr *, FunctionDecl *, FunctionProtoType::ExtParameterInfo, Qualifiers>(
       NumArgs, EPI.Variadic, EPI.requiresFunctionProtoTypeExtraBitfields(),
-      ESH.NumExceptionType, ESH.NumExprPtr, ESH.NumFunctionDeclPtr,
+      EPI.requiresFunctionProtoTypeArmAttributes(), ESH.NumExceptionType,
+      ESH.NumExprPtr, ESH.NumFunctionDeclPtr,
       EPI.ExtParameterInfos ? NumArgs : 0,
       EPI.TypeQuals.hasNonFastQualifiers() ? 1 : 0);
 
diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
index b419fc8836b032..3db5ae182f32c4 100644
--- a/clang/lib/AST/Type.cpp
+++ b/clang/lib/AST/Type.cpp
@@ -3459,6 +3459,14 @@ FunctionProtoType::FunctionProtoType(QualType result, ArrayRef<QualType> params,
     FunctionTypeBits.HasExtraBitfields = false;
   }
 
+  if (epi.requiresFunctionProtoTypeArmAttributes()) {
+    auto &ArmTypeAttrs = *getTrailingObjects<FunctionTypeArmAttributes>();
+    ArmTypeAttrs = FunctionTypeArmAttributes();
+
+    // Also set the bit in FunctionTypeExtraBitfields
+    auto &ExtraBits = *getTrailingObjects<FunctionTypeExtraBitfields>();
+    ExtraBits.HasArmTypeAttributes = true;
+  }
 
   // Fill in the trailing argument array.
   auto *argSlot = getTrailingObjects<QualType>();
@@ -3470,10 +3478,10 @@ FunctionProtoType::FunctionProtoType(QualType result, ArrayRef<QualType> params,
 
   // Propagate the SME ACLE attributes.
   if (epi.AArch64SMEAttributes != SME_NormalFunction) {
-    auto &ExtraBits = *getTrailingObjects<FunctionTypeExtraBitfields>();
+    auto &ArmTypeAttrs = *getTrailingObjects<FunctionTypeArmAttributes>();
     assert(epi.AArch64SMEAttributes <= SME_AttributeMask &&
            "Not enough bits to encode SME attributes");
-    ExtraBits.AArch64SMEAttributes = epi.AArch64SMEAttributes;
+    ArmTypeAttrs.AArch64SMEAttributes = epi.AArch64SMEAttributes;
   }
 
   // Fill in the exception type array if present.

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way of moving these further away is alright with me. If Aaron is OK with it, than I am too. So please wait for his approval too.

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you!

@sdesmalen-arm sdesmalen-arm merged commit d24f23e into llvm:main Jan 18, 2024
6 checks passed
ampandey-1995 pushed a commit to ampandey-1995/llvm-project that referenced this pull request Jan 19, 2024
…lvm#78424)

This decouples the Arm type attributes from other bits, which means
the data will only be allocated when a function uses these Arm
attributes.

The first patch adds the bit `HasArmTypeAttributes` to
`FunctionTypeBitfields`, which grows from 62 bits to 63 bits.

In the second patch, I've moved this bit (`HasArmTypeAttributes`) to
`FunctionTypeExtraBitfields`, because it looks like the bits in
`FunctionTypeBitfields` are precious and we really don't want that
struct
to grow beyond 64 bits.

I've split this out into two patches to explain the rationale, but those
can be squashed before merging.
@sdesmalen-arm sdesmalen-arm deleted the clang-refactor-sme-attr-bits branch February 23, 2024 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants