Skip to content

[mlir][nfc] Rename type constraint for scalable vectors #68808

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

Merged
merged 2 commits into from
Oct 13, 2023

Conversation

banach-space
Copy link
Contributor

@banach-space banach-space commented Oct 11, 2023

For consistency with other predicates, rename:

  • allDimsScalableVectorTypePred -> IsVectorTypeWithAllDimsScalablePred
  • IsScalableVectorTypePred -> IsVectorTypeWithAnyDimScalablePred

For consistency with other predicates, rename:
  * allDimsScalableVectorTypePred -> IsVectorTypeWithAllDimsScalablePred
@llvmbot
Copy link
Member

llvmbot commented Oct 11, 2023

@llvm/pr-subscribers-mlir-sme
@llvm/pr-subscribers-mlir-core
@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-ods

Author: Andrzej Warzyński (banach-space)

Changes

For consistency with other predicates, rename:

  • allDimsScalableVectorTypePred -> IsVectorTypeWithAllDimsScalablePred

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

2 Files Affected:

  • (modified) mlir/include/mlir/Dialect/ArmSME/IR/ArmSMEOps.td (+1-1)
  • (modified) mlir/include/mlir/IR/CommonTypeConstraints.td (+1-1)
diff --git a/mlir/include/mlir/Dialect/ArmSME/IR/ArmSMEOps.td b/mlir/include/mlir/Dialect/ArmSME/IR/ArmSMEOps.td
index e09092268082dd3..049c9759d70bf43 100644
--- a/mlir/include/mlir/Dialect/ArmSME/IR/ArmSMEOps.td
+++ b/mlir/include/mlir/Dialect/ArmSME/IR/ArmSMEOps.td
@@ -27,7 +27,7 @@ include "mlir/Interfaces/InferTypeOpInterface.td"
 
 class SMETileType<Type datatype, list<int> dims, string description>
   : ShapedContainerType<[datatype],
-      And<[IsVectorOfRankPred<[2]>, allDimsScalableVectorTypePred,
+      And<[IsVectorOfRankPred<[2]>, IsVectorTypeWithAllDimsScalablePred,
            IsVectorOfShape<dims>]>,
   description>;
 
diff --git a/mlir/include/mlir/IR/CommonTypeConstraints.td b/mlir/include/mlir/IR/CommonTypeConstraints.td
index 4fc14e30b8a10d0..00dcc66a576f148 100644
--- a/mlir/include/mlir/IR/CommonTypeConstraints.td
+++ b/mlir/include/mlir/IR/CommonTypeConstraints.td
@@ -38,7 +38,7 @@ def IsScalableVectorTypePred : CPred<[{::llvm::isa<::mlir::VectorType>($_self) &
                                    ::llvm::cast<VectorType>($_self).isScalable()}]>;
 
 // Whether a type is a VectorType and all dimensions are scalable.
-def allDimsScalableVectorTypePred : And<[
+def IsVectorTypeWithAllDimsScalablePred : And<[
   IsVectorTypePred,
   CPred<[{::llvm::cast<::mlir::VectorType>($_self).allDimsScalable()}]>
 ]>;

@@ -27,7 +27,7 @@ include "mlir/Interfaces/InferTypeOpInterface.td"

class SMETileType<Type datatype, list<int> dims, string description>
: ShapedContainerType<[datatype],
And<[IsVectorOfRankPred<[2]>, allDimsScalableVectorTypePred,
And<[IsVectorOfRankPred<[2]>, IsVectorTypeWithAllDimsScalablePred,
Copy link
Member

@MacDue MacDue Oct 11, 2023

Choose a reason for hiding this comment

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

Not required: The IsVectorOfRankPred<[2]> is redundant as the dims list always has two elements.

Suggested change
And<[IsVectorOfRankPred<[2]>, IsVectorTypeWithAllDimsScalablePred,
And<[IsVectorTypeWithAllDimsScalablePred,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha, interesting! I will keep it as a useful bit of documentation.

Copy link
Collaborator

@c-rhodes c-rhodes left a comment

Choose a reason for hiding this comment

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

LGTM, I wonder if IsScalableVectorTypePred should also be renamed to IsVectorTypeWithAnyDimScalablePred for clarity

@banach-space
Copy link
Contributor Author

IsVectorTypeWithAnyDimScalablePred

+1 Let me update this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants