AArch64: fix regression introduced by fcmp immediate selection.

Forgot to check if the predicate is safe to commutate operands.
jaebaek · Jan 16, 2021 · 8456c3a · 8456c3a
1 parent a61e42e
commit 8456c3a
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 8 deletions.
diff --git a/llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp b/llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
@@ -34,6 +34,7 @@
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/TargetOpcodes.h"
 #include "llvm/IR/Constants.h"
+#include "llvm/IR/Instructions.h"
 #include "llvm/IR/PatternMatch.h"
 #include "llvm/IR/Type.h"
 #include "llvm/IR/IntrinsicsAArch64.h"
@@ -177,8 +178,10 @@ class AArch64InstructionSelector : public InstructionSelector {
                                    MachineIRBuilder &MIRBuilder) const;
 
   /// Emit a floating point comparison between \p LHS and \p RHS.
+  /// \p Pred if given is the intended predicate to use.
   MachineInstr *emitFPCompare(Register LHS, Register RHS,
-                              MachineIRBuilder &MIRBuilder) const;
+                              MachineIRBuilder &MIRBuilder,
+                              Optional<CmpInst::Predicate> = None) const;
 
   MachineInstr *emitInstr(unsigned Opcode,
                           std::initializer_list<llvm::DstOp> DstOps,
@@ -1483,11 +1486,11 @@ bool AArch64InstructionSelector::selectCompareBranchFedByFCmp(
   assert(I.getOpcode() == TargetOpcode::G_BRCOND);
   // Unfortunately, the mapping of LLVM FP CC's onto AArch64 CC's isn't
   // totally clean.  Some of them require two branches to implement.
-  emitFPCompare(FCmp.getOperand(2).getReg(), FCmp.getOperand(3).getReg(), MIB);
+  auto Pred = (CmpInst::Predicate)FCmp.getOperand(1).getPredicate();
+  emitFPCompare(FCmp.getOperand(2).getReg(), FCmp.getOperand(3).getReg(), MIB,
+                Pred);
   AArch64CC::CondCode CC1, CC2;
-  changeFCMPPredToAArch64CC(
-      static_cast<CmpInst::Predicate>(FCmp.getOperand(1).getPredicate()), CC1,
-      CC2);
+  changeFCMPPredToAArch64CC(static_cast<CmpInst::Predicate>(Pred), CC1, CC2);
   MachineBasicBlock *DestMBB = I.getOperand(1).getMBB();
   MIB.buildInstr(AArch64::Bcc, {}, {}).addImm(CC1).addMBB(DestMBB);
   if (CC2 != AArch64CC::AL)
@@ -3090,7 +3093,7 @@ bool AArch64InstructionSelector::select(MachineInstr &I) {
     CmpInst::Predicate Pred =
         static_cast<CmpInst::Predicate>(I.getOperand(1).getPredicate());
     if (!emitFPCompare(I.getOperand(2).getReg(), I.getOperand(3).getReg(),
-                       MIRBuilder) ||
+                       MIRBuilder, Pred) ||
         !emitCSetForFCmp(I.getOperand(0).getReg(), Pred, MIRBuilder))
       return false;
     I.eraseFromParent();
@@ -4211,7 +4214,8 @@ MachineInstr *AArch64InstructionSelector::emitCSetForFCmp(
 
 MachineInstr *
 AArch64InstructionSelector::emitFPCompare(Register LHS, Register RHS,
-                                          MachineIRBuilder &MIRBuilder) const {
+                                          MachineIRBuilder &MIRBuilder,
+                                          Optional<CmpInst::Predicate> Pred) const {
   MachineRegisterInfo &MRI = *MIRBuilder.getMRI();
   LLT Ty = MRI.getType(LHS);
   if (Ty.isVector())
@@ -4224,7 +4228,12 @@ AArch64InstructionSelector::emitFPCompare(Register LHS, Register RHS,
   // to explicitly materialize a constant.
   const ConstantFP *FPImm = getConstantFPVRegVal(RHS, MRI);
   bool ShouldUseImm = FPImm && (FPImm->isZero() && !FPImm->isNegative());
-  if (!ShouldUseImm) {
+
+  auto IsEqualityPred = [](CmpInst::Predicate P) {
+    return P == CmpInst::FCMP_OEQ || P == CmpInst::FCMP_ONE ||
+           P == CmpInst::FCMP_UEQ || P == CmpInst::FCMP_UNE;
+  };
+  if (!ShouldUseImm && Pred && IsEqualityPred(*Pred)) {
     // Try commutating the operands.
     const ConstantFP *LHSImm = getConstantFPVRegVal(LHS, MRI);
     if (LHSImm && (LHSImm->isZero() && !LHSImm->isNegative())) {

diff --git a/llvm/test/CodeGen/AArch64/GlobalISel/select-fcmp.mir b/llvm/test/CodeGen/AArch64/GlobalISel/select-fcmp.mir
@@ -134,3 +134,29 @@ body:             |
     RET_ReallyLR implicit $s0
 
 ...
+---
+name:            zero_lhs_not_commutative_pred
+alignment:       4
+legalized:       true
+regBankSelected: true
+tracksRegLiveness: true
+body:             |
+  bb.1:
+    liveins: $s0, $s1
+
+    ; CHECK-LABEL: name: zero_lhs_not_commutative_pred
+    ; CHECK: liveins: $s0, $s1
+    ; CHECK: [[COPY:%[0-9]+]]:fpr32 = COPY $s0
+    ; CHECK: [[FMOVS0_:%[0-9]+]]:fpr32 = FMOVS0
+    ; CHECK: FCMPSrr [[FMOVS0_]], [[COPY]], implicit-def $nzcv
+    ; CHECK: [[CSINCWr:%[0-9]+]]:gpr32 = CSINCWr $wzr, $wzr, 5, implicit $nzcv
+    ; CHECK: $s0 = COPY [[CSINCWr]]
+    ; CHECK: RET_ReallyLR implicit $s0
+    %0:fpr(s32) = COPY $s0
+    %1:fpr(s32) = COPY $s1
+    %2:fpr(s32) = G_FCONSTANT float 0.000000e+00
+    %3:gpr(s32) = G_FCMP floatpred(olt), %2(s32), %0
+    $s0 = COPY %3(s32)
+    RET_ReallyLR implicit $s0
+
+...