Skip to content

Conversation

@AZero13
Copy link
Contributor

@AZero13 AZero13 commented Aug 25, 2025

CMN also has a function like this, we should do the same with CMP.

@llvmbot
Copy link
Member

llvmbot commented Aug 25, 2025

@llvm/pr-subscribers-backend-aarch64

Author: AZero13 (AZero13)

Changes

CMN also has a function like this, we should do the same with CMP.


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

1 Files Affected:

  • (modified) llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp (+14-5)
diff --git a/llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp b/llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
index 0bceb322726d1..edec7ac5ae47e 100644
--- a/llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
+++ b/llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
@@ -310,6 +310,8 @@ class AArch64InstructionSelector : public InstructionSelector {
                          MachineIRBuilder &MIRBuilder) const;
   MachineInstr *emitSBCS(Register Dst, MachineOperand &LHS, MachineOperand &RHS,
                          MachineIRBuilder &MIRBuilder) const;
+  MachineInstr *emitCMP(MachineOperand &LHS, MachineOperand &RHS,
+                        MachineIRBuilder &MIRBuilder) const;
   MachineInstr *emitCMN(MachineOperand &LHS, MachineOperand &RHS,
                         MachineIRBuilder &MIRBuilder) const;
   MachineInstr *emitTST(MachineOperand &LHS, MachineOperand &RHS,
@@ -4414,6 +4416,15 @@ AArch64InstructionSelector::emitSBCS(Register Dst, MachineOperand &LHS,
   return emitInstr(OpcTable[Is32Bit], {Dst}, {LHS, RHS}, MIRBuilder);
 }
 
+MachineInstr *
+AArch64InstructionSelector::emitCMP(MachineOperand &LHS, MachineOperand &RHS,
+                                    MachineIRBuilder &MIRBuilder) const {
+  MachineRegisterInfo &MRI = MIRBuilder.getMF().getRegInfo();
+  bool Is32Bit = (MRI.getType(LHS.getReg()).getSizeInBits() == 32);
+  auto RC = Is32Bit ? &AArch64::GPR32RegClass : &AArch64::GPR64RegClass;
+  return emitSUBS(MRI.createVirtualRegister(RC), LHS, RHS, MIRBuilder);
+}
+
 MachineInstr *
 AArch64InstructionSelector::emitCMN(MachineOperand &LHS, MachineOperand &RHS,
                                     MachineIRBuilder &MIRBuilder) const {
@@ -4466,8 +4477,7 @@ MachineInstr *AArch64InstructionSelector::emitIntegerCompare(
   // Fold the compare into a cmn or tst if possible.
   if (auto FoldCmp = tryFoldIntegerCompare(LHS, RHS, Predicate, MIRBuilder))
     return FoldCmp;
-  auto Dst = MRI.cloneVirtualRegister(LHS.getReg());
-  return emitSUBS(Dst, LHS, RHS, MIRBuilder);
+  return emitCMP(LHS, RHS, MIRBuilder);
 }
 
 MachineInstr *AArch64InstructionSelector::emitCSetForFCmp(
@@ -4874,9 +4884,8 @@ MachineInstr *AArch64InstructionSelector::emitConjunctionRec(
     if (!CCOp) {
       auto Dst = MRI.cloneVirtualRegister(LHS);
       if (isa<GICmp>(Cmp))
-        return emitSUBS(Dst, Cmp->getOperand(2), Cmp->getOperand(3), MIB);
-      return emitFPCompare(Cmp->getOperand(2).getReg(),
-                           Cmp->getOperand(3).getReg(), MIB);
+        return emitCMP(Cmp->getOperand(2), Cmp->getOperand(3), MIB);
+      return emitFPCompare(LHS, RHS, MIB, CC);
     }
     // Otherwise produce a ccmp.
     return emitConditionalComparison(LHS, RHS, CC, Predicate, OutCC, MIB);

… every time (NFC)

CMN also has a function like this, we should do the same with CMP.
@AZero13
Copy link
Contributor Author

AZero13 commented Nov 4, 2025

@MacDue Thoughts?

@AZero13 AZero13 requested a review from davemgreen November 7, 2025 19:52
@AZero13
Copy link
Contributor Author

AZero13 commented Nov 7, 2025

@davemgreen Done!

Copy link
Collaborator

@davemgreen davemgreen left a comment

Choose a reason for hiding this comment

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

Thanks. You could do it in these other emit.. functions too if you wanted.

LGTM.

@AZero13
Copy link
Contributor Author

AZero13 commented Nov 10, 2025

Thanks. You could do it in these other emit.. functions too if you wanted.

LGTM.

Thank you. Can you please merge?

@AZero13 AZero13 requested a review from davemgreen November 11, 2025 15:20
@davemgreen
Copy link
Collaborator

This is failing tests but a local rerun went OK.

@davemgreen davemgreen merged commit d6703bb into llvm:main Nov 13, 2025
8 of 10 checks passed
@AZero13 AZero13 deleted the emitcmp branch November 13, 2025 17:13
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.

3 participants