[PowerPC] Fix assert from machine verify pass that unmatched register class about fcmp selection in fast-isel

[• wuzish]

Summary

The way to reproduce the issue:

llc -mtriple powerpc64le-unknown-linux-gnu -fast-isel -O0 < llvm/test/CodeGen/PowerPC/fast-isel-fcmp-nan.ll -verify-machineinstrs

Bad machine code: Illegal virtual register for instruction

function: TestULE
basic block: %bb.0 entry (0x1000a39b158)
instruction: %2:crrc = FCMPUD %1:vsfrc, %3:f8rc
operand 1: %1:vsfrc

Fix assert about missing match between fcmp instruction and register class. We should use vsx related cmp instruction xvcmpudp instead of fcmpu when vsx is opened.

add -verifymachineinstrs option into related test cases to enable the verify pass.

@wschmidt could you please have a look since the last change is related to your part? Thanks.

[• wuzish]

Gentle ping...

[• wuzish]

Gentle ping again

[hfinkel]

llvm/lib/Target/PowerPC/PPCFastISel.cpp

900 ↗

(On Diff #178167)

This would be the only place I see here where we're checking hasVSX(). Should we call getRegForValue earlier and then use isVSFRCRegClass? Do we need to do the same thing about with isVSSRCRegClass and PPC::FCMPUS?

[nemanjai]

I assume that we are fixing this specific instance because we have a test case that breaks if we turn on verification by default. However, I would assume that we actually have a lot of other failures hiding in here which would be uncovered by doing a bootstrap build with -O0. I am not suggesting that we put all the fixes in a single patch, just that we want to do more thorough testing before we turn on MachineFunction verification by default.

llvm/lib/Target/PowerPC/PPCFastISel.cpp

900 ↗

(On Diff #178167)

I would actually suggest we go one step further and leave the switch the way it is but change the opcode after we create the virtual registers based on whether the Src... VRegs are VSSRC/VSFRC. This would also allow us to add an assert if we ever hit a situation where the register classes don't match (i.e. one is VSFRC and the other F8RC, etc.).

[• wuzish]

Address comment

[• wuzish]

llvm/lib/Target/PowerPC/PPCFastISel.cpp

900 ↗

(On Diff #178167)

I find it's relative easy and OK to move the getRegForValue earlier. It's hard to make an assert of "one is VSFRC and the other F8RC" since the src2 is an imm result materialized by a load which is a normal f8rc load instead of vsx load instruction, which I think is a limitation now since we don't handle load selection well.

[• wuzish]

Gentle ping for new update

[hfinkel]

llvm/lib/Target/PowerPC/PPCFastISel.cpp

895 ↗

(On Diff #179991)

It's best to write this out : "Unsupposed f32 VSX comparison".

Otherwise, LGTM.

[• wuzish]

llvm/trunk/lib/Target/PowerPC/PPCFastISel.cpp

895

Well, it can not be assert. Just return false to let DAG selection handle it.

[• wuzish]

use copy to resolve the mismatch instead of assert.

llvm/trunk/lib/Target/PowerPC/PPCFastISel.cpp

if (SrcVT == MVT::i64 || SrcVT == MVT::i32 || SrcVT == MVT::i16 ||
         SrcVT == MVT::i8 || SrcVT == MVT::i1) {
       const APInt &CIVal = ConstInt->getValue();
       Imm = (IsZExt) ? (long)CIVal.getZExtValue() : (long)CIVal.getSExtValue();
       if ((IsZExt && isUInt<16>(Imm)) || (!IsZExt && isInt<16>(Imm)))
         UseImm = true;
     }
   }
 
+  unsigned SrcReg1 = getRegForValue(SrcValue1);
+  if (SrcReg1 == 0)
+    return false;
+
+  unsigned SrcReg2 = 0;
+  if (!UseImm) {
+    SrcReg2 = getRegForValue(SrcValue2);
+    if (SrcReg2 == 0)
+      return false;
+  }
+
   unsigned CmpOpc;
   bool NeedsExt = false;
+  auto RC = MRI.getRegClass(SrcReg1);
   switch (SrcVT.SimpleTy) {
     default: return false;
     case MVT::f32:
       if (HasSPE) {
         switch (Pred) {
           default: return false;
           case PPC::PRED_EQ:
             CmpOpc = PPC::EFSCMPEQ;
             break;
           case PPC::PRED_LT:
             CmpOpc = PPC::EFSCMPLT;
             break;
           case PPC::PRED_GT:
             CmpOpc = PPC::EFSCMPGT;
             break;
         }
-      } else
+      } else if (isVSSRCRegClass(RC)) {
+        llvm_unreachable("Unsupposed f32 VSX comparison");
+      } else {
         CmpOpc = PPC::FCMPUS;
+      }
       break;
     case MVT::f64:
       if (HasSPE) {
         switch (Pred) {
           default: return false;
           case PPC::PRED_EQ:
             CmpOpc = PPC::EFDCMPEQ;
             break;
           case PPC::PRED_LT:
             CmpOpc = PPC::EFDCMPLT;
             break;
           case PPC::PRED_GT:
             CmpOpc = PPC::EFDCMPGT;
             break;
         }
-      } else
+      } else if (isVSFRCRegClass(RC)) {
+        CmpOpc = PPC::XSCMPUDP;
+      } else {
         CmpOpc = PPC::FCMPUD;
+      }
       break;
     case MVT::i1:
     case MVT::i8:
     case MVT::i16:
       NeedsExt = true;
       LLVM_FALLTHROUGH;
     case MVT::i32:
       if (!UseImm)
         CmpOpc = IsZExt ? PPC::CMPLW : PPC::CMPW;
       else
         CmpOpc = IsZExt ? PPC::CMPLWI : PPC::CMPWI;
       break;
     case MVT::i64:
       if (!UseImm)
         CmpOpc = IsZExt ? PPC::CMPLD : PPC::CMPD;
       else
         CmpOpc = IsZExt ? PPC::CMPLDI : PPC::CMPDI;
       break;
   }
 
-  unsigned SrcReg1 = getRegForValue(SrcValue1);
-  if (SrcReg1 == 0)
-    return false;
-
-  unsigned SrcReg2 = 0;
-  if (!UseImm) {
-    SrcReg2 = getRegForValue(SrcValue2);
-    if (SrcReg2 == 0)
-      return false;
-  }
-
   if (NeedsExt) {
     unsigned ExtReg = createResultReg(&PPC::GPRCRegClass);
     if (!PPCEmitIntExt(SrcVT, SrcReg1, MVT::i32, ExtReg, IsZExt))
       return false;
     SrcReg1 = ExtReg;
 
     if (!UseImm) {
       unsigned ExtReg = createResultReg(&PPC::GPRCRegClass);

llvm/trunk/test/CodeGen/PowerPC/fast-isel-fcmp-nan.ll

-; RUN: llc -mtriple powerpc64le-unknown-linux-gnu -fast-isel -O0 < %s | FileCheck %s
+; RUN: llc -verify-machineinstrs -mtriple powerpc64le-unknown-linux-gnu -fast-isel -O0 < %s | FileCheck %s
 
 define i1 @TestULT(double %t0) {
 ; CHECK-LABEL: TestULT:
 ; CHECK: xscmpudp
 ; CHECK: blr
 entry:
   %t1 = fcmp ult double %t0, 0.000000e+00
   br i1 %t1, label %good, label %bad
 
 bad:
   ret i1 false
 
 good:
   ret i1 true
 }
 
 define i1 @TestULE(double %t0) {
 ; CHECK-LABEL: TestULE:
-; CHECK: fcmpu
+; CHECK: xscmpudp
 ; CHECK-NEXT: ble
 ; CHECK: blr
 entry:
   %t1 = fcmp ule double %t0, 0.000000e+00
   br i1 %t1, label %good, label %bad
 
 bad:
   ret i1 false
 
 good:
   ret i1 true
 }
 
 define i1 @TestUNE(double %t0) {
 ; CHECK-LABEL: TestUNE:
-; CHECK: fcmpu
+; CHECK: xscmpudp
 ; CHECK-NEXT: bne
 ; CHECK: blr
 entry:
   %t1 = fcmp une double %t0, 0.000000e+00
   br i1 %t1, label %good, label %bad
 
 bad:
   ret i1 false
bad:
   ret i1 false
 
 good:
   ret i1 true
 }
 
 define i1 @TestUGE(double %t0) {
 ; CHECK-LABEL: TestUGE:
-; CHECK: fcmpu
+; CHECK: xscmpudp
 ; CHECK-NEXT: bge
 ; CHECK: blr
 entry:
   %t1 = fcmp uge double %t0, 0.000000e+00
   br i1 %t1, label %good, label %bad
 
 bad:
   ret i1 false
 
 good:
   ret i1 true
 }
 
 define i1 @TestOLT(double %t0) {
 ; CHECK-LABEL: TestOLT:
-; CHECK: fcmpu
+; CHECK: xscmpudp
 ; CHECK-NEXT: blt
 ; CHECK: blr
 entry:
   %t1 = fcmp olt double %t0, 0.000000e+00
   br i1 %t1, label %good, label %bad
 
 bad:
   ret i1 false
bad:
   ret i1 false
 
 good:
   ret i1 true
 }
 
 define i1 @TestOEQ(double %t0) {
 ; CHECK-LABEL: TestOEQ:
-; CHECK: fcmpu
+; CHECK: xscmpudp
 ; CHECK-NEXT: beq
 ; CHECK: blr
 entry:
   %t1 = fcmp oeq double %t0, 0.000000e+00
   br i1 %t1, label %good, label %bad
 
 bad:
   ret i1 false
 
 good:
   ret i1 true
 }
 
 define i1 @TestOGT(double %t0) {
 ; CHECK-LABEL: TestOGT:
-; CHECK: fcmpu
+; CHECK: xscmpudp
 ; CHECK-NEXT: bgt
 ; CHECK: blr
 entry:
   %t1 = fcmp ogt double %t0, 0.000000e+00
   br i1 %t1, label %good, label %bad
 
 bad:
   ret i1 false

llvm/trunk/test/CodeGen/PowerPC/vsx-self-copy.ll

-; RUN: llc -mcpu=pwr7 -mattr=+vsx < %s | FileCheck %s
-; RUN: llc -mcpu=pwr7 -mattr=+vsx -fast-isel -O0 < %s | FileCheck %s
+; RUN: llc -mcpu=pwr7 -mattr=+vsx < %s -verify-machineinstrs | FileCheck %s
+; RUN: llc -mcpu=pwr7 -mattr=+vsx -fast-isel -O0 < %s -verify-machineinstrs | FileCheck %s
 target datalayout = "E-m:e-i64:64-n32:64"
 target triple = "powerpc64-unknown-linux-gnu"
 
 define double @takFP(double %x, double %y, double %z) #0 {
 entry:
   br i1 undef, label %if.then, label %return
 
 if.then:                                          ; preds = %if.then, %entry