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

[TableGen] Replace all lingering uses of getName with getEnumName #113731

Conversation

jrtc27
Copy link
Collaborator

@jrtc27 jrtc27 commented Oct 25, 2024

The former is a wrapper for the latter with two differences: Other is
mapped to "UNKNOWN" (rather than "MVT::Other"), and iPTR(Any) are mapped
to "TLI.getPointerTy()" rather than "MVT::iPTR(Any)".

The only uses are in FastISelMap::printFunctionDefinitions. Most of
these uses are just a form of name mangling to ensure uniqueness, so the
actual string isn't important (and, in the case of MVT::iPTR(Any), were
both to be used, they would clash). Two uses are for a case statement,
which requires the expression to be a constant (of the right type), but
neither UNKNOWN nor TLI.getPointerTy() are constants, so would not work
there. The remaining uses are where an expression is needed, so UNKNOWN
similarly doesn't work, though TLI.getPointerTy() could in this case.
However, neither iPTR nor iPTRAny are supposed to make it this far
through TableGen, and should instead have been replaced with concrete
types, so this case should not be hit. Moreover, for almost all of these
uses, the name is passed to getLegalCName, which will strip an MVT::
prefix but will leave TLI.getPointerTy() unchanged, which is not a valid
C identifier, nor component thereof.

Thus, delete this unnecessary, and mostly-broken, wrapper and just use
the underlying getEnumName. This has been verified to have no effect on
the generated files for any in-tree target, including experimental ones.

Created using spr 1.3.4
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 25, 2024

@llvm/pr-subscribers-tablegen

Author: Jessica Clarke (jrtc27)

Changes

The former is a wrapper for the latter with two differences: Other is
mapped to "UNKNOWN" (rather than "MVT::Other"), and iPTR(Any) are mapped
to "TLI.getPointerTy()" rather than "MVT::iPTR(Any)".

The only uses are in FastISelMap::printFunctionDefinitions. Most of
these uses are just a form of name mangling to ensure uniqueness, so the
actual string isn't important (and, in the case of MVT::iPTR(Any), were
both to be used, they would clash). Two uses are for a case statement,
which requires the expression to be a constant (of the right type), but
neither UNKNOWN nor TLI.getPointerTy() are constants, so would not work
there. The remaining uses are where an expression is needed, so UNKNOWN
similarly doesn't work, though TLI.getPointerTy() could in this case.
However, neither iPTR nor iPTRAny are supposed to make it this far
through TableGen, and should instead have been replaced with concrete
types, so this case should not be hit. Moreover, for almost all of these
uses, the name is passed to getLegalCName, which will strip an MVT::
prefix but will leave TLI.getPointerTy() unchanged, which is not a valid
C identifier, nor component thereof.

Thus, delete this unnecessary, and mostly-broken, wrapper and just use
the underlying getEnumName. This has been verified to have no effect on
the generated files for any in-tree target, including experimental ones.


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

3 Files Affected:

  • (modified) llvm/utils/TableGen/Common/CodeGenTarget.cpp (-13)
  • (modified) llvm/utils/TableGen/Common/CodeGenTarget.h (-1)
  • (modified) llvm/utils/TableGen/FastISelEmitter.cpp (+10-10)
diff --git a/llvm/utils/TableGen/Common/CodeGenTarget.cpp b/llvm/utils/TableGen/Common/CodeGenTarget.cpp
index b358518c4290b0..4e75db689a0b57 100644
--- a/llvm/utils/TableGen/Common/CodeGenTarget.cpp
+++ b/llvm/utils/TableGen/Common/CodeGenTarget.cpp
@@ -47,19 +47,6 @@ MVT::SimpleValueType llvm::getValueType(const Record *Rec) {
   return (MVT::SimpleValueType)Rec->getValueAsInt("Value");
 }
 
-StringRef llvm::getName(MVT::SimpleValueType T) {
-  switch (T) {
-  case MVT::Other:
-    return "UNKNOWN";
-  case MVT::iPTR:
-    return "TLI.getPointerTy()";
-  case MVT::iPTRAny:
-    return "TLI.getPointerTy()";
-  default:
-    return getEnumName(T);
-  }
-}
-
 StringRef llvm::getEnumName(MVT::SimpleValueType T) {
   // clang-format off
   switch (T) {
diff --git a/llvm/utils/TableGen/Common/CodeGenTarget.h b/llvm/utils/TableGen/Common/CodeGenTarget.h
index c7b44f7028eb5b..8bcb2f677a00b0 100644
--- a/llvm/utils/TableGen/Common/CodeGenTarget.h
+++ b/llvm/utils/TableGen/Common/CodeGenTarget.h
@@ -46,7 +46,6 @@ class CodeGenSubRegIndex;
 /// record corresponds to.
 MVT::SimpleValueType getValueType(const Record *Rec);
 
-StringRef getName(MVT::SimpleValueType T);
 StringRef getEnumName(MVT::SimpleValueType T);
 
 /// getQualifiedName - Return the name of the specified record, with a
diff --git a/llvm/utils/TableGen/FastISelEmitter.cpp b/llvm/utils/TableGen/FastISelEmitter.cpp
index 17198c85f06009..c73060b3c59884 100644
--- a/llvm/utils/TableGen/FastISelEmitter.cpp
+++ b/llvm/utils/TableGen/FastISelEmitter.cpp
@@ -718,19 +718,19 @@ void FastISelMap::printFunctionDefinitions(raw_ostream &OS) {
             const PredMap &PM = RI.second;
 
             OS << "unsigned fastEmit_" << getLegalCName(Opcode) << "_"
-               << getLegalCName(std::string(getName(VT))) << "_"
-               << getLegalCName(std::string(getName(RetVT))) << "_";
+               << getLegalCName(std::string(getEnumName(VT))) << "_"
+               << getLegalCName(std::string(getEnumName(RetVT))) << "_";
             Operands.PrintManglingSuffix(OS, ImmediatePredicates);
             OS << "(";
             Operands.PrintParameters(OS);
             OS << ") {\n";
 
-            emitInstructionCode(OS, Operands, PM, std::string(getName(RetVT)));
+            emitInstructionCode(OS, Operands, PM, std::string(getEnumName(RetVT)));
           }
 
           // Emit one function for the type that demultiplexes on return type.
           OS << "unsigned fastEmit_" << getLegalCName(Opcode) << "_"
-             << getLegalCName(std::string(getName(VT))) << "_";
+             << getLegalCName(std::string(getEnumName(VT))) << "_";
           Operands.PrintManglingSuffix(OS, ImmediatePredicates);
           OS << "(MVT RetVT";
           if (!Operands.empty())
@@ -739,10 +739,10 @@ void FastISelMap::printFunctionDefinitions(raw_ostream &OS) {
           OS << ") {\nswitch (RetVT.SimpleTy) {\n";
           for (const auto &RI : RM) {
             MVT::SimpleValueType RetVT = RI.first;
-            OS << "  case " << getName(RetVT) << ": return fastEmit_"
+            OS << "  case " << getEnumName(RetVT) << ": return fastEmit_"
                << getLegalCName(Opcode) << "_"
-               << getLegalCName(std::string(getName(VT))) << "_"
-               << getLegalCName(std::string(getName(RetVT))) << "_";
+               << getLegalCName(std::string(getEnumName(VT))) << "_"
+               << getLegalCName(std::string(getEnumName(RetVT))) << "_";
             Operands.PrintManglingSuffix(OS, ImmediatePredicates);
             OS << "(";
             Operands.PrintArguments(OS);
@@ -753,7 +753,7 @@ void FastISelMap::printFunctionDefinitions(raw_ostream &OS) {
         } else {
           // Non-variadic return type.
           OS << "unsigned fastEmit_" << getLegalCName(Opcode) << "_"
-             << getLegalCName(std::string(getName(VT))) << "_";
+             << getLegalCName(std::string(getEnumName(VT))) << "_";
           Operands.PrintManglingSuffix(OS, ImmediatePredicates);
           OS << "(MVT RetVT";
           if (!Operands.empty())
@@ -761,7 +761,7 @@ void FastISelMap::printFunctionDefinitions(raw_ostream &OS) {
           Operands.PrintParameters(OS);
           OS << ") {\n";
 
-          OS << "  if (RetVT.SimpleTy != " << getName(RM.begin()->first)
+          OS << "  if (RetVT.SimpleTy != " << getEnumName(RM.begin()->first)
              << ")\n    return 0;\n";
 
           const PredMap &PM = RM.begin()->second;
@@ -781,7 +781,7 @@ void FastISelMap::printFunctionDefinitions(raw_ostream &OS) {
       OS << "  switch (VT.SimpleTy) {\n";
       for (const auto &TI : TM) {
         MVT::SimpleValueType VT = TI.first;
-        std::string TypeName = std::string(getName(VT));
+        std::string TypeName = std::string(getEnumName(VT));
         OS << "  case " << TypeName << ": return fastEmit_"
            << getLegalCName(Opcode) << "_" << getLegalCName(TypeName) << "_";
         Operands.PrintManglingSuffix(OS, ImmediatePredicates);

Copy link

github-actions bot commented Oct 25, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Created using spr 1.3.4
@jrtc27 jrtc27 requested a review from arsenm October 29, 2024 16:50
jrtc27 added a commit to jrtc27/llvm-llvm-project that referenced this pull request Oct 29, 2024
The former is a wrapper for the latter with two differences: Other is
mapped to "UNKNOWN" (rather than "MVT::Other"), and iPTR(Any) are mapped
to "TLI.getPointerTy()" rather than "MVT::iPTR(Any)".

The only uses are in FastISelMap::printFunctionDefinitions. Most of
these uses are just a form of name mangling to ensure uniqueness, so the
actual string isn't important (and, in the case of MVT::iPTR(Any), were
both to be used, they would clash). Two uses are for a case statement,
which requires the expression to be a constant (of the right type), but
neither UNKNOWN nor TLI.getPointerTy() are constants, so would not work
there. The remaining uses are where an expression is needed, so UNKNOWN
similarly doesn't work, though TLI.getPointerTy() could in this case.
However, neither iPTR nor iPTRAny are supposed to make it this far
through TableGen, and should instead have been replaced with concrete
types, so this case should not be hit. Moreover, for almost all of these
uses, the name is passed to getLegalCName, which will strip an MVT::
prefix but will leave TLI.getPointerTy() unchanged, which is not a valid
C identifier, nor component thereof.

Thus, delete this unnecessary, and mostly-broken, wrapper and just use
the underlying getEnumName. This has been verified to have no effect on
the generated files for any in-tree target, including experimental ones.

Pull Request: llvm#113731
@jrtc27 jrtc27 merged commit ef455e6 into main Oct 30, 2024
8 checks passed
@jrtc27 jrtc27 deleted the users/jrtc27/spr/tablegen-replace-all-lingering-uses-of-getname-with-getenumname branch October 30, 2024 03:12
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.

3 participants