[ValueTypes] Remove MVT::MAX_ALLOWED_VALUETYPE. NFC #93654

topperc · 2024-05-29T06:51:53Z

Despite the comment, this isn't used to size bit vectors or tables. That's done by VALUETYPE_SIZE. MAX_ALLOWED_VALUETYPE is only used by some static_asserts that compare it to VALUETYPE_SIZE.

This patch removes it and most of the static_asserts. I left one where I compared VALUETYPE_SIZE to token which is the first type that isn't part of the VALUETYPE range. This isn't strictly needed, we'd probably catch duplication error from VTEmitter.cpp first.

I've removed the hardcoded 224 that was present in tablegen by adding a new bit to the ValueType class to mark value types that should be excluded from the VALUETYPE range.

While here I've converted the 'is*' flags to bits and used getValueAsBit instead of getValueAsInt. I can make this a separate change if desired.

llvmbot · 2024-05-29T06:52:27Z

@llvm/pr-subscribers-llvm-selectiondag

Author: Craig Topper (topperc)

Changes

Despite the comment, this isn't used to size bit vectors or tables. That's done by VALUETYPE_SIZE. MAX_ALLOWED_VALUETYPE is only used by some static_asserts that compare it to VALUETYPE_SIZE.

This patch removes it and most of the static_asserts. I left one where I compared VALUETYPE_SIZE to token which is the first type that isn't part of the VALUETYPE range. This isn't strictly needed, we'd probably catch duplication error from VTEmitter.cpp first.

I've removed the hardcoded 224 that was present in tablegen by adding a new bit to the ValueType class to mark value types that should be excluded from the VALUETYPE range.

While here I've converted the 'is*' flags to bits and used getValueAsBit instead of getValueAsInt. I can make this a separate change if desired.

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

5 Files Affected:

(modified) llvm/include/llvm/CodeGen/ValueTypes.td (+9-5)
(modified) llvm/include/llvm/CodeGenTypes/MachineValueType.h (+1-6)
(modified) llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h (-2)
(modified) llvm/lib/CodeGen/TargetLoweringBase.cpp (-3)
(modified) llvm/utils/TableGen/VTEmitter.cpp (+9-8)

diff --git a/llvm/include/llvm/CodeGen/ValueTypes.td b/llvm/include/llvm/CodeGen/ValueTypes.td
index e322cc04c1c76..c7f9650ec5b56 100644
--- a/llvm/include/llvm/CodeGen/ValueTypes.td
+++ b/llvm/include/llvm/CodeGen/ValueTypes.td
@@ -18,11 +18,12 @@ class ValueType<int size, int value> {
   int Value = value;
   int nElem = 1;
   ValueType ElementType = ?;
-  int isOverloaded = false;
-  int isInteger = false;
-  int isFP = false;
-  int isVector = false;
-  int isScalable = false;
+  bit isOverloaded = false;
+  bit isInteger = false;
+  bit isFP = false;
+  bit isVector = false;
+  bit isScalable = false;
+  bit isNormalValueType = true;
 }
 
 class VTAny<int value> : ValueType<0, value> {
@@ -287,6 +288,7 @@ def aarch64svcount
               : ValueType<16,  199>;  // AArch64 predicate-as-counter
 def spirvbuiltin : ValueType<0, 200>; // SPIR-V's builtin type
 
+let isNormalValueType = false in {
 def token      : ValueType<0, 248>;  // TokenTy
 def MetadataVT : ValueType<0, 249> { // Metadata
   let LLVMName = "Metadata";
@@ -316,6 +318,8 @@ def iPTR       : ValueType<0, 254>;
 // Should only be used in TableGen.
 def Any        : VTAny<255>;
 
+} // isNormalValueType = false
+
 } // end defset ValueTypes
 
 /// This class is for targets that want to use pointer types in patterns
diff --git a/llvm/include/llvm/CodeGenTypes/MachineValueType.h b/llvm/include/llvm/CodeGenTypes/MachineValueType.h
index 3b2a9b535c094..e008503f734b9 100644
--- a/llvm/include/llvm/CodeGenTypes/MachineValueType.h
+++ b/llvm/include/llvm/CodeGenTypes/MachineValueType.h
@@ -45,15 +45,10 @@ namespace llvm {
 #undef GET_VT_RANGES
 
       VALUETYPE_SIZE = LAST_VALUETYPE + 1,
-
-      // This is the current maximum for LAST_VALUETYPE.
-      // MVT::MAX_ALLOWED_VALUETYPE is used for asserts and to size bit vectors
-      // This value must be a multiple of 32.
-      MAX_ALLOWED_VALUETYPE = 224,
     };
 
     static_assert(FIRST_VALUETYPE > 0);
-    static_assert(LAST_VALUETYPE < MAX_ALLOWED_VALUETYPE);
+    static_assert(LAST_VALUETYPE < token);
 
     SimpleValueType SimpleTy = INVALID_SIMPLE_VALUE_TYPE;
 
diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h b/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
index ba3c7582d5a8a..bec9cb49b5864 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
@@ -168,8 +168,6 @@ class LLVM_LIBRARY_VISIBILITY DAGTypeLegalizer {
   explicit DAGTypeLegalizer(SelectionDAG &dag)
     : TLI(dag.getTargetLoweringInfo()), DAG(dag),
     ValueTypeActions(TLI.getValueTypeActions()) {
-    static_assert(MVT::LAST_VALUETYPE <= MVT::MAX_ALLOWED_VALUETYPE,
-                  "Too many value types for ValueTypeActions to hold!");
   }
 
   /// This is the main entry point for the type legalizer.  This does a
diff --git a/llvm/lib/CodeGen/TargetLoweringBase.cpp b/llvm/lib/CodeGen/TargetLoweringBase.cpp
index 82a59918b085b..f2e4632b248f4 100644
--- a/llvm/lib/CodeGen/TargetLoweringBase.cpp
+++ b/llvm/lib/CodeGen/TargetLoweringBase.cpp
@@ -1430,9 +1430,6 @@ TargetLoweringBase::findRepresentativeClass(const TargetRegisterInfo *TRI,
 /// this allows us to compute derived properties we expose.
 void TargetLoweringBase::computeRegisterProperties(
     const TargetRegisterInfo *TRI) {
-  static_assert(MVT::VALUETYPE_SIZE <= MVT::MAX_ALLOWED_VALUETYPE,
-                "Too many value types for ValueTypeActions to hold!");
-
   // Everything defaults to needing one register.
   for (unsigned i = 0; i != MVT::VALUETYPE_SIZE; ++i) {
     NumRegistersForVT[i] = 1;
diff --git a/llvm/utils/TableGen/VTEmitter.cpp b/llvm/utils/TableGen/VTEmitter.cpp
index 5ec1f59318f78..64b54ed134232 100644
--- a/llvm/utils/TableGen/VTEmitter.cpp
+++ b/llvm/utils/TableGen/VTEmitter.cpp
@@ -68,10 +68,11 @@ void VTEmitter::run(raw_ostream &OS) {
       continue;
     auto Name = VT->getValueAsString("LLVMName");
     auto Value = VT->getValueAsInt("Value");
-    bool IsInteger = VT->getValueAsInt("isInteger");
-    bool IsFP = VT->getValueAsInt("isFP");
-    bool IsVector = VT->getValueAsInt("isVector");
-    bool IsScalable = VT->getValueAsInt("isScalable");
+    bool IsInteger = VT->getValueAsBit("isInteger");
+    bool IsFP = VT->getValueAsBit("isFP");
+    bool IsVector = VT->getValueAsBit("isVector");
+    bool IsScalable = VT->getValueAsBit("isScalable");
+    bool IsNormalValueType =  VT->getValueAsBit("isNormalValueType");
 
     UpdateVTRange("INTEGER_FIXEDLEN_VECTOR_VALUETYPE", Name,
                   IsInteger && IsVector && !IsScalable);
@@ -85,14 +86,14 @@ void VTEmitter::run(raw_ostream &OS) {
     UpdateVTRange("VECTOR_VALUETYPE", Name, IsVector);
     UpdateVTRange("INTEGER_VALUETYPE", Name, IsInteger && !IsVector);
     UpdateVTRange("FP_VALUETYPE", Name, IsFP && !IsVector);
-    UpdateVTRange("VALUETYPE", Name, Value < 224);
+    UpdateVTRange("VALUETYPE", Name, IsNormalValueType);
 
     // clang-format off
     OS << "  GET_VT_ATTR("
        << Name << ", "
        << Value << ", "
        << VT->getValueAsInt("Size") << ", "
-       << VT->getValueAsInt("isOverloaded") << ", "
+       << VT->getValueAsBit("isOverloaded") << ", "
        << (IsInteger ? Name[0] == 'i' ? 3 : 1 : 0) << ", "
        << (IsFP ? Name[0] == 'f' ? 3 : 1 : 0) << ", "
        << IsVector << ", "
@@ -111,14 +112,14 @@ void VTEmitter::run(raw_ostream &OS) {
 
   OS << "#ifdef GET_VT_VECATTR // (Ty, Sc, nElem, ElTy, ElSz)\n";
   for (const auto *VT : VTsByNumber) {
-    if (!VT || !VT->getValueAsInt("isVector"))
+    if (!VT || !VT->getValueAsBit("isVector"))
       continue;
     const auto *ElTy = VT->getValueAsDef("ElementType");
     assert(ElTy);
     // clang-format off
     OS << "  GET_VT_VECATTR("
        << VT->getValueAsString("LLVMName") << ", "
-       << VT->getValueAsInt("isScalable") << ", "
+       << VT->getValueAsBit("isScalable") << ", "
        << VT->getValueAsInt("nElem") << ", "
        << ElTy->getName() << ", "
        << ElTy->getValueAsInt("Size") << ")\n";

github-actions · 2024-05-29T06:54:41Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 1de6011c34b185235cd65c2e3fb030015d182968 b5129828c3fec0413428196522fe9279b914c593 -- llvm/include/llvm/CodeGenTypes/MachineValueType.h llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h llvm/lib/CodeGen/TargetLoweringBase.cpp

View the diff from clang-format here.

diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h b/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
index bec9cb49b5..ef54484e93 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
@@ -166,9 +166,8 @@ private:
 
 public:
   explicit DAGTypeLegalizer(SelectionDAG &dag)
-    : TLI(dag.getTargetLoweringInfo()), DAG(dag),
-    ValueTypeActions(TLI.getValueTypeActions()) {
-  }
+      : TLI(dag.getTargetLoweringInfo()), DAG(dag),
+        ValueTypeActions(TLI.getValueTypeActions()) {}
 
   /// This is the main entry point for the type legalizer.  This does a
   /// top-down traversal of the dag, legalizing types as it goes.  Returns

chapuni

Thanks. Reasonable.
Could you wait for one more opinions?

I can make this a separate change if desired.

I prefer it may be split in advance. It is a good cleanup.

arsenm · 2024-05-29T08:03:47Z

llvm/utils/TableGen/VTEmitter.cpp

+    bool IsInteger = VT->getValueAsBit("isInteger");
+    bool IsFP = VT->getValueAsBit("isFP");
+    bool IsVector = VT->getValueAsBit("isVector");
+    bool IsScalable = VT->getValueAsBit("isScalable");
+    bool IsNormalValueType =  VT->getValueAsBit("isNormalValueType");


Can just submit this separately

Add a new bit to ValueTypes.td to indicate whether a type should be part of the [FIRST_VALUETYPE,LAST_VALUETYPE] range or not. This was reviewed as part of #93654.

Despite the comment, this isn't used to size bit vectors or tables. That's done by VALUETYPE_SIZE. MAX_ALLOWED_VALUETYPE is only used by some static_asserts that compare it to VALUETYPE_SIZE. This patch removes it and most of the static_asserts. I left one where I compared VALUETYPE_SIZE to token which is the first type that isn't part of the VALUETYPE range. This isn't strictly needed, we'd probably catch duplication error from VTEmitter.cpp first. I've removed the hardcoded 224 that was present in tablegen by adding a new bit to the ValueType class to mark value types that should be excluded from the VALUETYPE range. While here I've converted the 'is*' flags to bits and used getValueAsBit instead of getValueAsInt. I can make this a separate change if desired.

topperc requested review from arsenm, gchatelet, chapuni and RKSimon May 29, 2024 06:51

llvmbot added the llvm:SelectionDAG SelectionDAGISel as well label May 29, 2024

chapuni approved these changes May 29, 2024

View reviewed changes

arsenm approved these changes May 29, 2024

View reviewed changes

topperc force-pushed the pr/max_allowed_valuetype branch from 860aa20 to b512982 Compare May 29, 2024 16:02

topperc merged commit 8aceb7a into llvm:main May 29, 2024

topperc deleted the pr/max_allowed_valuetype branch May 29, 2024 18:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ValueTypes] Remove MVT::MAX_ALLOWED_VALUETYPE. NFC #93654

[ValueTypes] Remove MVT::MAX_ALLOWED_VALUETYPE. NFC #93654

Uh oh!

topperc commented May 29, 2024

Uh oh!

llvmbot commented May 29, 2024

Uh oh!

github-actions bot commented May 29, 2024 •

edited

Loading

Uh oh!

chapuni left a comment

Uh oh!

arsenm May 29, 2024

Uh oh!

Uh oh!

[ValueTypes] Remove MVT::MAX_ALLOWED_VALUETYPE. NFC #93654

[ValueTypes] Remove MVT::MAX_ALLOWED_VALUETYPE. NFC #93654

Uh oh!

Conversation

topperc commented May 29, 2024

Uh oh!

llvmbot commented May 29, 2024

Uh oh!

github-actions bot commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chapuni left a comment

Choose a reason for hiding this comment

Uh oh!

arsenm May 29, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented May 29, 2024 •

edited

Loading