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][RISCV] Remove duplicate functions isRVVSizelessBuiltinType. NFC #67089

Closed
wants to merge 1 commit into from

Conversation

eopXD
Copy link
Member

@eopXD eopXD commented Sep 22, 2023

isRVVSizelessBuiltinType and isRVVType has the same functionality. This commit removes the former since we have more variants available in isRVVType.

@eopXD eopXD requested review from topperc and a team September 22, 2023 06:25
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Sep 22, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 22, 2023

@llvm/pr-subscribers-clang

Changes

isRVVSizelessBuiltinType and isRVVType has the same functionality. This commit removes the former since we have more variants available in isRVVType.


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

5 Files Affected:

  • (modified) clang/include/clang/AST/Type.h (-3)
  • (modified) clang/lib/AST/ASTContext.cpp (+6-8)
  • (modified) clang/lib/AST/Type.cpp (+1-14)
  • (modified) clang/lib/Sema/SemaExpr.cpp (+4-4)
  • (modified) clang/lib/Sema/SemaOverload.cpp (+1-2)
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index 4799f89db82fa7f..f4eb57e19d9370d 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -2061,9 +2061,6 @@ class alignas(8) Type : public ExtQualsTypeCommonBase {
   /// Returns true for SVE scalable vector types.
   bool isSVESizelessBuiltinType() const;
 
-  /// Returns true for RVV scalable vector types.
-  bool isRVVSizelessBuiltinType() const;
-
   /// Check if this is a WebAssembly Externref Type.
   bool isWebAssemblyExternrefType() const;
 
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 4b1d9e86797b778..c670624b21f14ec 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -9571,10 +9571,9 @@ static uint64_t getRVVTypeSize(ASTContext &Context, const BuiltinType *Ty) {
 
 bool ASTContext::areCompatibleRVVTypes(QualType FirstType,
                                        QualType SecondType) {
-  assert(
-      ((FirstType->isRVVSizelessBuiltinType() && SecondType->isVectorType()) ||
-       (FirstType->isVectorType() && SecondType->isRVVSizelessBuiltinType())) &&
-      "Expected RVV builtin type and vector type!");
+  assert(((FirstType->isRVVType() && SecondType->isVectorType()) ||
+          (FirstType->isVectorType() && SecondType->isRVVType())) &&
+         "Expected RVV builtin type and vector type!");
 
   auto IsValidCast = [this](QualType FirstType, QualType SecondType) {
     if (const auto *BT = FirstType->getAs<BuiltinType>()) {
@@ -9596,10 +9595,9 @@ bool ASTContext::areCompatibleRVVTypes(QualType FirstType,
 
 bool ASTContext::areLaxCompatibleRVVTypes(QualType FirstType,
                                           QualType SecondType) {
-  assert(
-      ((FirstType->isRVVSizelessBuiltinType() && SecondType->isVectorType()) ||
-       (FirstType->isVectorType() && SecondType->isRVVSizelessBuiltinType())) &&
-      "Expected RVV builtin type and vector type!");
+  assert(((FirstType->isRVVType() && SecondType->isVectorType()) ||
+          (FirstType->isVectorType() && SecondType->isRVVType())) &&
+         "Expected RVV builtin type and vector type!");
 
   auto IsLaxCompatible = [this](QualType FirstType, QualType SecondType) {
     const auto *BT = FirstType->getAs<BuiltinType>();
diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
index c08ebfb7f142b35..d0be891122c8fc9 100644
--- a/clang/lib/AST/Type.cpp
+++ b/clang/lib/AST/Type.cpp
@@ -2372,7 +2372,7 @@ bool Type::isIncompleteType(NamedDecl **Def) const {
 }
 
 bool Type::isSizelessBuiltinType() const {
-  if (isSVESizelessBuiltinType() || isRVVSizelessBuiltinType())
+  if (isSVESizelessBuiltinType() || isRVVType())
     return true;
 
   if (const BuiltinType *BT = getAs<BuiltinType>()) {
@@ -2420,19 +2420,6 @@ bool Type::isSVESizelessBuiltinType() const {
   return false;
 }
 
-bool Type::isRVVSizelessBuiltinType() const {
-  if (const BuiltinType *BT = getAs<BuiltinType>()) {
-    switch (BT->getKind()) {
-#define RVV_TYPE(Name, Id, SingletonId) case BuiltinType::Id:
-#include "clang/Basic/RISCVVTypes.def"
-      return true;
-    default:
-      return false;
-    }
-  }
-  return false;
-}
-
 bool Type::isSveVLSBuiltinType() const {
   if (const BuiltinType *BT = getAs<BuiltinType>()) {
     switch (BT->getKind()) {
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 92496b03ecabe54..dd1080a1085f07c 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -8223,7 +8223,7 @@ bool Sema::isValidRVVBitcast(QualType srcTy, QualType destTy) {
   assert(srcTy->isVectorType() || destTy->isVectorType());
 
   auto ValidScalableConversion = [](QualType FirstType, QualType SecondType) {
-    if (!FirstType->isRVVSizelessBuiltinType())
+    if (!FirstType->isRVVType())
       return false;
 
     const auto *VecTy = SecondType->getAs<VectorType>();
@@ -10212,8 +10212,8 @@ Sema::CheckAssignmentConstraints(QualType LHSType, ExprResult &RHS,
       }
 
     // Allow assignments between fixed-length and sizeless RVV vectors.
-    if ((LHSType->isRVVSizelessBuiltinType() && RHSType->isVectorType()) ||
-        (LHSType->isVectorType() && RHSType->isRVVSizelessBuiltinType())) {
+    if ((LHSType->isRVVType() && RHSType->isVectorType()) ||
+        (LHSType->isVectorType() && RHSType->isRVVType())) {
       if (Context.areCompatibleRVVTypes(LHSType, RHSType) ||
           Context.areLaxCompatibleRVVTypes(LHSType, RHSType)) {
         Kind = CK_BitCast;
@@ -11161,7 +11161,7 @@ QualType Sema::CheckVectorOperands(ExprResult &LHS, ExprResult &RHS,
         SecondVecType->getVectorKind() == VectorType::GenericVector) {
       if (FirstType->isSVESizelessBuiltinType())
         return true;
-      if (FirstType->isRVVSizelessBuiltinType()) {
+      if (FirstType->isRVVType()) {
         SVEorRVV = 1;
         return true;
       }
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 0ac2ac258c0c7d1..4919641dd44acde 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -1785,8 +1785,7 @@ static bool IsVectorConversion(Sema &S, QualType FromType, QualType ToType,
       return true;
     }
 
-  if (ToType->isRVVSizelessBuiltinType() ||
-      FromType->isRVVSizelessBuiltinType())
+  if (ToType->isRVVType() || FromType->isRVVType())
     if (S.Context.areCompatibleRVVTypes(FromType, ToType) ||
         S.Context.areLaxCompatibleRVVTypes(FromType, ToType)) {
       ICK = ICK_RVV_Vector_Conversion;

@eopXD
Copy link
Member Author

eopXD commented Oct 2, 2023

Ping.

1 similar comment
@eopXD
Copy link
Member Author

eopXD commented Nov 16, 2023

Ping.

`isRVVSizelessBuiltinType` and `isRVVType` has the same functionality.
This commit removes the former since we have more variants available in
`isRVVType`.
Copy link
Collaborator

@topperc topperc left a comment

Choose a reason for hiding this comment

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

LGTM

@topperc
Copy link
Collaborator

topperc commented Dec 9, 2023

Can we reverse this patch? Keep isRVVSizelessBuiltinType and remove isRVVType. The implementation of isRVVSizelessBuiltinType is more efficient according to some profiling. At least on Release+Asserts build

@topperc
Copy link
Collaborator

topperc commented Dec 9, 2023

Can we reverse this patch? Keep isRVVSizelessBuiltinType and remove isRVVType. The implementation of isRVVSizelessBuiltinType is more efficient according to some profiling. At least on Release+Asserts build

I went ahead and committed that change.

@cor3ntin
Copy link
Contributor

cor3ntin commented May 3, 2024

Closing that as an alternative solution was committed

@cor3ntin cor3ntin closed this May 3, 2024
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