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

Remove the DREGS_WITHOUT_Z hack #126

Open
dylanmckay opened this issue Jan 16, 2019 · 2 comments
Open

Remove the DREGS_WITHOUT_Z hack #126

dylanmckay opened this issue Jan 16, 2019 · 2 comments

Comments

@dylanmckay
Copy link
Member

This was introduced in #37 in order to work around a regalloc bug.

Since then, a root cause fix has landed in upstream LLVM in D54218.

Nirav pointed this out in the bugzilla associated with this issue; https://bugs.llvm.org/show_bug.cgi?id=38224.

Now that the root cause has been fixed, we should be able to remove the DREGS_WITHOUT_Z_WORKAROUND register class introduced in 9de46729ce2fb4bf4b2a19b7b387898f256d827f, which will give us more optimal code generation.

@dsprenkels
Copy link

dsprenkels commented Mar 30, 2019

I was just messing around a bit. Removing the DREGS_WITHOUT_Z hack (in upstream llvm) still seems to trigger the tests CodeGen/AVR/rust-avr-bug-37.ll and CodeGen/AVR/rust-avr-bug-95.ll.


That is:

commit 3f33b241bd1d7cd77fb89d058f6355668ea758d1 (HEAD -> avr-rust-126, origin/avr-rust-126)
Author: Daan Sprenkels <[email protected]>
Date:   Sat Mar 30 22:38:12 2019 +0100

    [AVR] Remove the DREGS_WITHOUT_Z hack
    
    As proposed by https://github.com/avr-rust/rust/issues/126

diff --git a/llvm/lib/Target/AVR/AVRInstrInfo.td b/llvm/lib/Target/AVR/AVRInstrInfo.td
index c458fe7de06..5669d88b25e 100644
--- a/llvm/lib/Target/AVR/AVRInstrInfo.td
+++ b/llvm/lib/Target/AVR/AVRInstrInfo.td
@@ -1230,7 +1230,7 @@ isReMaterializable = 1 in
   // ldd Rd,   P+q
   // ldd Rd+1, P+q+1
   let Constraints = "@earlyclobber $dst" in
-  def LDDWRdPtrQ : Pseudo<(outs DREGS_WITHOUT_Z_WORKAROUND:$dst),
+  def LDDWRdPtrQ : Pseudo<(outs DREGS:$dst),
                           (ins memri:$memri),
                           "lddw\t$dst, $memri",
                           [(set i16:$dst, (load addr:$memri))]>,
diff --git a/llvm/lib/Target/AVR/AVRRegisterInfo.td b/llvm/lib/Target/AVR/AVRRegisterInfo.td
index e20f69beabe..b83f0597dd9 100644
--- a/llvm/lib/Target/AVR/AVRRegisterInfo.td
+++ b/llvm/lib/Target/AVR/AVRRegisterInfo.td
@@ -156,26 +156,6 @@ def DREGS : RegisterClass<"AVR", [i16], 8,
     R9R8, R7R6, R5R4, R3R2, R1R0
   )>;
 
-// The 16-bit DREGS register class, excluding the Z pointer register.
-//
-// This is used by instructions which cause high pointer register
-// contention which leads to an assertion in the register allocator.
-//
-// There is no technical reason why instructions that use this class
-// cannot use Z; it's simply a workaround a regalloc bug.
-//
-// More information can be found in PR39553.
-def DREGS_WITHOUT_Z_WORKAROUND : RegisterClass<"AVR", [i16], 8,
-  (
-    // Return value and arguments.
-    add R25R24, R19R18, R21R20, R23R22,
-    // Scratch registers.
-    R27R26,
-    // Callee saved registers.
-    R29R28, R17R16, R15R14, R13R12, R11R10,
-    R9R8, R7R6, R5R4, R3R2, R1R0
-  )>;
-
 // 16-bit register class for immediate instructions.
 def DLDREGS : RegisterClass<"AVR", [i16], 8,
   (

gives:

Failing Tests (3):
    LLVM :: CodeGen/AVR/issue-cannot-select-bswap.ll
    LLVM :: CodeGen/AVR/rust-avr-bug-37.ll
    LLVM :: CodeGen/AVR/rust-avr-bug-95.ll

  Expected Passes    : 119
  Expected Failures  : 3
  Unexpected Failures: 3

(Note: CodeGen/AVR/issue-cannot-select-bswap.ll also failed before applying the patch.)

@aykevl
Copy link

aykevl commented Jan 20, 2022

It looks like this workaround is not necessary anymore, so I have made a patch to remove it: https://reviews.llvm.org/D117831
All CodeGen tests pass with this workaround removed.

aykevl added a commit to llvm/llvm-project that referenced this issue Jan 23, 2022
Background: avr-rust/rust-legacy-fork#126

In short, this workaround was introduced to fix a "ran out of registers
during regalloc" issue. The root cause has since been fixed in
https://reviews.llvm.org/D54218 so this workaround can be removed.

There is one test that changes a little bit, removing a single
instruction. I also compiled compiler-rt before and after this patch but
didn't see a difference. So presumably the impact is very low. Still,
it's nice to be able to remove such a workaround.

Differential Revision: https://reviews.llvm.org/D117831
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
Background: avr-rust/rust-legacy-fork#126

In short, this workaround was introduced to fix a "ran out of registers
during regalloc" issue. The root cause has since been fixed in
https://reviews.llvm.org/D54218 so this workaround can be removed.

There is one test that changes a little bit, removing a single
instruction. I also compiled compiler-rt before and after this patch but
didn't see a difference. So presumably the impact is very low. Still,
it's nice to be able to remove such a workaround.

Differential Revision: https://reviews.llvm.org/D117831
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants