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

[ARM] Resolve FIXME: Swap adds <-> subs offset is 0 #78870

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AtariDreams
Copy link
Contributor

@AtariDreams AtariDreams commented Jan 21, 2024

Also erase instruction if offset is 0.

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 21, 2024

@llvm/pr-subscribers-backend-arm

Author: AtariDreams (AtariDreams)

Changes

Also erase instruction if offset is 0


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

1 Files Affected:

  • (modified) llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp (+13-7)
diff --git a/llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp b/llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp
index ed9d30c3c3ab90..10478611497a08 100644
--- a/llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp
+++ b/llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp
@@ -525,13 +525,19 @@ void ARMLoadStoreOpt::UpdateBaseRegUses(MachineBasicBlock &MBB,
         // Merge it with the update; if the merged offset is too large,
         // insert a new sub instead.
         MachineOperand &MO =
-          MBBI->getOperand(MBBI->getDesc().getNumOperands() - 3);
-        Offset = (Opc == ARM::tSUBi8) ?
-          MO.getImm() + WordOffset * 4 :
-          MO.getImm() - WordOffset * 4 ;
-        if (Offset >= 0 && TL->isLegalAddImmediate(Offset)) {
-          // FIXME: Swap ADDS<->SUBS if Offset < 0, erase instruction if
-          // Offset == 0.
+            MBBI->getOperand(MBBI->getDesc().getNumOperands() - 3);
+        Offset = (Opc == ARM::tSUBi8) ? MO.getImm() + WordOffset * 4
+                                      : MO.getImm() - WordOffset * 4;
+        if (TL->isLegalAddImmediate(Offset)) {
+          if (Offset < 0) {
+            // Swap ADDS<->SUBS if Offset < 0
+            Opc = (Opc == ARM::tSUBi8) ? ARM::tADDi8 : ARM::tSUBi8;
+            Offset = -Offset;
+          } else if (Offset == 0) {
+            // Erase instruction if Offset == 0
+            MBBI = MBB.erase(MBBI);
+            return;
+          }
           MO.setImm(Offset);
           // The base register has now been reset, so exit early.
           return;

Copy link

github-actions bot commented Jan 21, 2024

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

@davemgreen
Copy link
Collaborator

Do you have tests cases for where this can come up? I'm not sure we should be generating instructions with negative offsets.
Could the add/sub do setting nzcv flags that are needed?

@AtariDreams AtariDreams changed the title Resolve FIXME: Swap adds <-> subs offset is 0 [ARM] Resolve FIXME: Swap adds <-> subs offset is 0 Feb 7, 2024
Also erase instruction if offset is 0.
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

3 participants