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

[CodeGen] Don't include aliases in RegisterClassInfo::IgnoreCSRForAllocOrder #80015

Merged
merged 1 commit into from
Jan 31, 2024

Conversation

jayfoad
Copy link
Contributor

@jayfoad jayfoad commented Jan 30, 2024

Previously we called ignoreCSRForAllocationOrder on every alias of every
CSR which was expensive on targets like AMDGPU which define a very large
number of overlapping register tuples.

On such targets it is simpler and faster to call
ignoreCSRForAllocationOrder once for every physical register.

Differential Revision: https://reviews.llvm.org/D146735

…ocOrder

Previously we called ignoreCSRForAllocationOrder on every alias of every
CSR which was expensive on targets like AMDGPU which define a very large
number of overlapping register tuples.

On such targets it is simpler and faster to call
ignoreCSRForAllocationOrder once for every physical register.

Differential Revision: https://reviews.llvm.org/D146735
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 30, 2024

@llvm/pr-subscribers-llvm-regalloc

Author: Jay Foad (jayfoad)

Changes

Previously we called ignoreCSRForAllocationOrder on every alias of every
CSR which was expensive on targets like AMDGPU which define a very large
number of overlapping register tuples.

On such targets it is simpler and faster to call
ignoreCSRForAllocationOrder once for every physical register.

Differential Revision: https://reviews.llvm.org/D146735


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

1 Files Affected:

  • (modified) llvm/lib/CodeGen/RegisterClassInfo.cpp (+3-5)
diff --git a/llvm/lib/CodeGen/RegisterClassInfo.cpp b/llvm/lib/CodeGen/RegisterClassInfo.cpp
index 8869a861de06..1dd595bc140c 100644
--- a/llvm/lib/CodeGen/RegisterClassInfo.cpp
+++ b/llvm/lib/CodeGen/RegisterClassInfo.cpp
@@ -93,11 +93,9 @@ void RegisterClassInfo::runOnMachineFunction(const MachineFunction &mf) {
   // Even if CSR list is same, we could have had a different allocation order
   // if ignoreCSRForAllocationOrder is evaluated differently.
   BitVector CSRHintsForAllocOrder(TRI->getNumRegs());
-  for (const MCPhysReg *I = CSR; *I; ++I)
-    for (MCRegAliasIterator AI(*I, TRI, true); AI.isValid(); ++AI)
-      CSRHintsForAllocOrder[*AI] = STI.ignoreCSRForAllocationOrder(mf, *AI);
-  if (IgnoreCSRForAllocOrder.size() != CSRHintsForAllocOrder.size() ||
-      IgnoreCSRForAllocOrder != CSRHintsForAllocOrder) {
+  for (MCPhysReg I = 1, E = TRI->getNumRegs(); I != E; ++I)
+    CSRHintsForAllocOrder[I] = STI.ignoreCSRForAllocationOrder(mf, I);
+  if (IgnoreCSRForAllocOrder != CSRHintsForAllocOrder) {
     Update = true;
     IgnoreCSRForAllocOrder = CSRHintsForAllocOrder;
   }

@jayfoad
Copy link
Contributor Author

jayfoad commented Jan 30, 2024

Migrated from Phabricator. There is some discussion there: https://reviews.llvm.org/D146735

@jayfoad jayfoad merged commit f852503 into llvm:main Jan 31, 2024
4 of 5 checks passed
@nikic
Copy link
Contributor

nikic commented Jan 31, 2024

It looks like this approach is faster on AMDGPU, but slower on more conventional architectures: http://llvm-compile-time-tracker.com/compare.php?from=db49319264d6d2b6f9f7b345495d543210c2cfe3&to=f8525030004f907cd108e7c18df255a6d3b23124&stat=instructions:u

@jayfoad
Copy link
Contributor Author

jayfoad commented Jan 31, 2024

It looks like this approach is faster on AMDGPU, but slower on more conventional architectures:

Thanks for keeping me honest! I will revert and rethink.

jayfoad added a commit that referenced this pull request Jan 31, 2024
…SRForAllocOrder (#80015)"

This reverts commit f852503.

It was supposed to speed things up but llvm-compile-time-tracker.com
showed a slight slow down.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants