Skip to content

Conversation

resistor
Copy link
Collaborator

@resistor resistor commented Oct 3, 2025

Previously, the initialization of this table in CodeGenRegBank() indexed the table with the AsmName, while the only user of the table in CompressInstEmitter::validateRegister() indexed it using the name of the tblgen Def. Apparently these were the same essentially all of the time, so the issue never manifested. This patch standardizes on indexing using the tblgen def name, as that is guaranteed to be unique.

This issue impacted the forthcoming implementation of the RISC Y extension (CHERI), in which the capability registers have the same AsmName as the GPRs, causing llvm-tblgen assertion failures.

@resistor
Copy link
Collaborator Author

resistor commented Oct 3, 2025

@arichardson @jrtc27

@llvmbot
Copy link
Member

llvmbot commented Oct 3, 2025

@llvm/pr-subscribers-tablegen

Author: Owen Anderson (resistor)

Changes

Previously, the initialization of this table in CodeGenRegBank() indexed the table with the name of the tblgen def, while the only user of the table in CompressInstEmitter::validateRegister() indexed it using the name of the tblgen Def. Apparently these were the same essentially all of the time, so the issue never manifested. This patch standardizes on indexing using the tblgen def name, as that is guaranteed to be unique.

This issue impacted the forthcoming implementation of the RISC Y extension (CHERI), in which the capability registers have the same AsmName as the GPRs, causing llvm-tblgen assertion failures.


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

1 Files Affected:

  • (modified) llvm/utils/TableGen/Common/CodeGenRegisters.cpp (+1-1)
diff --git a/llvm/utils/TableGen/Common/CodeGenRegisters.cpp b/llvm/utils/TableGen/Common/CodeGenRegisters.cpp
index e873b3eaa4b7e..dd868cfa3eeae 100644
--- a/llvm/utils/TableGen/Common/CodeGenRegisters.cpp
+++ b/llvm/utils/TableGen/Common/CodeGenRegisters.cpp
@@ -1189,7 +1189,7 @@ CodeGenRegBank::CodeGenRegBank(const RecordKeeper &Records,
     // causes some failures in MIPS - perhaps they have duplicate register name
     // entries? (or maybe there's a reason for it - I don't know much about this
     // code, just drive-by refactoring)
-    RegistersByName.try_emplace(Reg.TheDef->getValueAsString("AsmName"), &Reg);
+    RegistersByName.try_emplace(Reg.getName().lower(), &Reg);
 
   // Precompute all sub-register maps.
   // This will create Composite entries for all inferred sub-register indices.

Previously, the initialization of this table in CodeGenRegBank() indexed the table with the name of the tblgen def, while the only user of the table in CompressInstEmitter::validateRegister() indexed it using the name of the tblgen Def. Apparently these were the same essentially all of the time, so the issue never manifested. This patch standardizes on indexing using the tblgen def name, as that is guaranteed to be unique.

This issue impacted the forthcoming implementation of the RISC Y extension (CHERI), in which the capability registers have the same AsmName as the GPRs, causing llvm-tblgen assertion failures.
@jayfoad
Copy link
Contributor

jayfoad commented Oct 3, 2025

Previously, the initialization of this table in CodeGenRegBank() indexed the table with the name of the tblgen def, while the only user of the table in CompressInstEmitter::validateRegister() indexed it using the name of the tblgen Def.

Er... what is the difference between "the name of the tblgen def" and "the name of the tblgen Def"?

@resistor
Copy link
Collaborator Author

resistor commented Oct 3, 2025

Previously, the initialization of this table in CodeGenRegBank() indexed the table with the name of the tblgen def, while the only user of the table in CompressInstEmitter::validateRegister() indexed it using the name of the tblgen Def.

Er... what is the difference between "the name of the tblgen def" and "the name of the tblgen Def"?

Sorry, that's an artifact of me revising the commit message. Will fix.

Originally one of them used the AsmName and the other used the Def's name.

@resistor
Copy link
Collaborator Author

resistor commented Oct 3, 2025

Unfortunately the X86 failures here appear to be real. I'll try to figure out what's wrong with it.

@resistor
Copy link
Collaborator Author

resistor commented Oct 3, 2025

Fixing the X86 resulted in a substantially different final change, so I'm closing this PR and opening a new one.

@resistor resistor closed this Oct 3, 2025
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