Skip to content

Conversation

resistor
Copy link
Collaborator

@resistor resistor commented Oct 3, 2025

The previous code was subtly incorrect, as it indexed the RegistersByName map using the tblgen Def name of the register, rather than the AsmName with which the table was initialized. But all of this indirection via the name was unnecessary.

…ssInstEmitter, rather than indirecting via the name.

The previous code was subtly incorrect, as it indexed the RegistersByName map using the tblgen Def name of the register, rather than the AsmName with which the table was initialized. But all of this indirection via the name was unnecessary.
@resistor
Copy link
Collaborator Author

resistor commented Oct 3, 2025

@jayfoad @jrtc27 @arichardson

@llvmbot
Copy link
Member

llvmbot commented Oct 3, 2025

@llvm/pr-subscribers-tablegen

Author: Owen Anderson (resistor)

Changes

The previous code was subtly incorrect, as it indexed the RegistersByName map using the tblgen Def name of the register, rather than the AsmName with which the table was initialized. But all of this indirection via the name was unnecessary.


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

1 Files Affected:

  • (modified) llvm/utils/TableGen/CompressInstEmitter.cpp (+1-1)
diff --git a/llvm/utils/TableGen/CompressInstEmitter.cpp b/llvm/utils/TableGen/CompressInstEmitter.cpp
index ccf83859924bc..d8c5ca7c1e1a3 100644
--- a/llvm/utils/TableGen/CompressInstEmitter.cpp
+++ b/llvm/utils/TableGen/CompressInstEmitter.cpp
@@ -167,7 +167,7 @@ bool CompressInstEmitter::validateRegister(const Record *Reg,
   assert(RegClass->isSubClassOf("RegisterClass") &&
          "RegClass record should be a RegisterClass");
   const CodeGenRegisterClass &RC = Target.getRegisterClass(RegClass);
-  const CodeGenRegister *R = Target.getRegisterByName(Reg->getName().lower());
+  const CodeGenRegister *R = Target.getRegBank().getReg(Reg);
   assert(R != nullptr && "Register not defined!!");
   return RC.contains(R);
 }

@arichardson arichardson requested a review from topperc October 3, 2025 21:46
Copy link
Member

@arichardson arichardson left a comment

Choose a reason for hiding this comment

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

Looks sensible to me but someone more familiar with this code should approve.

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

@resistor resistor merged commit f3703f3 into llvm:main Oct 4, 2025
9 checks passed
aokblast pushed a commit to aokblast/llvm-project that referenced this pull request Oct 6, 2025
…ssInstEmitter, rather than indirecting via the name. (llvm#161853)

The previous code was subtly incorrect, as it indexed the RegistersByName map using the tblgen Def name of the register, rather than the AsmName with which the table was initialized. But all of this indirection via the name was unnecessary.
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