[PowerPC] Change liveness testing in VSX FMA mutation pass

[wschmidt]

Summary

With VSX enabled, LLVM crashes when compiling test/CodeGen/PowerPC/fma.ll. I traced this to the liveness test that's removed in this patch. The interval test is designed to only work for virtual registers, but in this case the AddendSrcReg is physical. Since there is already a walk of the MIs between the AddendMI and the FMA, I added a check for def/kill of the AddendSrcReg in that loop and removed the extra liveness test.

There may well be a better way of doing this, so please redirect me if so...

Thanks,
Bill

[hfinkel]

LGTM.

lib/Target/PowerPC/PPCInstrInfo.cpp

1691

check for whether -> check whether

1724

Because I'm so trusting... please don't delete this; instead turn it into an assert that checks this condition (for virtual registers).

[wschmidt]

Thanks for the quick review! I've implemented the requested changes and committed the patch as r220090.

lib/Target/PowerPC/PPCInstrInfo.cpp

struct PPCVSXFMAMutate : public MachineFunctionPass {
     const PPCTargetMachine *TM;
     const PPCInstrInfo *TII;
 
 protected:
     bool processBlock(MachineBasicBlock &MBB) {
       bool Changed = false;
 
       MachineRegisterInfo &MRI = MBB.getParent()->getRegInfo();
+      const TargetRegisterInfo *TRI = &TII->getRegisterInfo();
       for (MachineBasicBlock::iterator I = MBB.begin(), IE = MBB.end();
            I != IE; ++I) {
         MachineInstr *MI = I;
 
         // The default (A-type) VSX FMA form kills the addend (it is taken from
         // the target register, which is then updated to reflect the result of
         // the FMA). If the instruction, however, kills one of the registers
         // used for the product, then we can use the M-form instruction (which
bool processBlock(MachineBasicBlock &MBB) {
           if (!MRI.getRegClass(AddendMI->getOperand(0).getReg())
                 ->contains(AddendSrcReg))
             continue;
         }
 
         // In theory, there could be other uses of the addend copy before this
         // fma.  We could deal with this, but that would require additional
         // logic below and I suspect it will not occur in any relevant
-        // situations.
-        bool OtherUsers = false;
+        // situations.  Additionally, check for whether the copy source is
+        // killed prior to the fma.  In order to replace the addend here with
+        // the source of the copy, it must still be live here.  We can't use
+        // interval testing for a physical register, so as long as we're
+        // walking the MIs we may as well test liveness here.
+        bool OtherUsers = false, KillsAddendSrc = false;
         for (auto J = std::prev(I), JE = MachineBasicBlock::iterator(AddendMI);
-             J != JE; --J)
+             J != JE; --J) {
           if (J->readsVirtualRegister(AddendMI->getOperand(0).getReg())) {
             OtherUsers = true;
             break;
           }
+          if (J->modifiesRegister(AddendSrcReg, TRI) ||
+              J->killsRegister(AddendSrcReg, TRI)) {
+            KillsAddendSrc = true;
+            break;
+          }
+        }
 
-        if (OtherUsers)
+        if (OtherUsers || KillsAddendSrc)
           continue;
 
         // Find one of the product operands that is killed by this instruction.
 
         unsigned KilledProdOp = 0, OtherProdOp = 0;
         if (LIS->getInterval(MI->getOperand(2).getReg())
                      .Query(FMAIdx).isKill()) {
           KilledProdOp = 2;
           OtherProdOp  = 3;
         } else if (LIS->getInterval(MI->getOperand(3).getReg())
                      .Query(FMAIdx).isKill()) {
           KilledProdOp = 3;
           OtherProdOp  = 2;
         }
 
         // If there are no killed product operands, then this transformation is
         // likely not profitable.
         if (!KilledProdOp)
           continue;
 
-        // In order to replace the addend here with the source of the copy,
-        // it must still be live here.
-        if (!LIS->getInterval(AddendMI->getOperand(1).getReg()).liveAt(FMAIdx))
-          continue;
-
         // Transform: (O2 * O3) + O1 -> (O2 * O1) + O3.
 
         unsigned AddReg = AddendMI->getOperand(1).getReg();
         unsigned KilledProdReg = MI->getOperand(KilledProdOp).getReg();
         unsigned OtherProdReg  = MI->getOperand(OtherProdOp).getReg();
 
         unsigned AddSubReg = AddendMI->getOperand(1).getSubReg();
         unsigned KilledProdSubReg = MI->getOperand(KilledProdOp).getSubReg();

test/CodeGen/PowerPC/fma.ll

-; RUN: llc < %s -march=ppc32 -fp-contract=fast | FileCheck %s
+; RUN: llc < %s -march=ppc32 -fp-contract=fast -mattr=-vsx | FileCheck %s
+; RUN: llc < %s -mtriple=powerpc64-unknown-linux-gnu -fp-contract=fast -mattr=+vsx | FileCheck -check-prefix=CHECK-VSX %s
 
 declare double @dummy1(double) #0
 declare double @dummy2(double, double) #0
 declare double @dummy3(double, double, double) #0
 
 define double @test_FMADD1(double %A, double %B, double %C) {
   %D = fmul double %A, %B   ; <double> [#uses=1]
   %E = fadd double %C, %D   ; <double> [#uses=1]
   ret double %E
 ; CHECK-LABEL: test_FMADD1:
 ; CHECK: fmadd
 ; CHECK-NEXT: blr
+
+; CHECK-VSX-LABEL: test_FMADD1:
+; CHECK-VSX: xsmaddmdp
+; CHECK-VSX-NEXT: blr
 }
 
 define double @test_FMADD2(double %A, double %B, double %C) {
   %D = fmul double %A, %B   ; <double> [#uses=1]
   %E = fadd double %D, %C   ; <double> [#uses=1]
   ret double %E
 ; CHECK-LABEL: test_FMADD2:
 ; CHECK: fmadd
 ; CHECK-NEXT: blr
+
+; CHECK-VSX-LABEL: test_FMADD2:
+; CHECK-VSX: xsmaddmdp
+; CHECK-VSX-NEXT: blr
 }
 
 define double @test_FMSUB1(double %A, double %B, double %C) {
   %D = fmul double %A, %B   ; <double> [#uses=1]
   %E = fsub double %D, %C   ; <double> [#uses=1]
   ret double %E
 ; CHECK-LABEL: test_FMSUB1:
 ; CHECK: fmsub
 ; CHECK-NEXT: blr
+
+; CHECK-VSX-LABEL: test_FMSUB1:
+; CHECK-VSX: xsmsubmdp
+; CHECK-VSX-NEXT: blr
 }
 
 define double @test_FMSUB2(double %A, double %B, double %C, double %D) {
   %E = fmul double %A, %B   ; <double> [#uses=2]
   %F = fadd double %E, %C   ; <double> [#uses=1]
   %G = fsub double %E, %D   ; <double> [#uses=1]
   %H = call double @dummy2(double %F, double %G)      ; <double> [#uses=1]
   ret double %H
 ; CHECK-LABEL: test_FMSUB2:
 ; CHECK: fmadd
 ; CHECK-NEXT: fmsub
+
+; CHECK-VSX-LABEL: test_FMSUB2:
+; CHECK-VSX: xsmaddadp
+; CHECK-VSX-NEXT: xsmsubmdp
 }
 
 define double @test_FNMADD1(double %A, double %B, double %C) {
   %D = fmul double %A, %B   ; <double> [#uses=1]
   %E = fadd double %D, %C   ; <double> [#uses=1]
   %F = fsub double -0.000000e+00, %E    ; <double> [#uses=1]
   ret double %F
 ; CHECK-LABEL: test_FNMADD1:
 ; CHECK: fnmadd
 ; CHECK-NEXT: blr
+
+; CHECK-VSX-LABEL: test_FNMADD1:
+; CHECK-VSX: xsnmaddmdp
+; CHECK-VSX-NEXT: blr
 }
 
 define double @test_FNMADD2(double %A, double %B, double %C) {
   %D = fmul double %A, %B   ; <double> [#uses=1]
   %E = fadd double %C, %D   ; <double> [#uses=1]
   %F = fsub double -0.000000e+00, %E    ; <double> [#uses=1]
   ret double %F
 ; CHECK-LABEL: test_FNMADD2:
 ; CHECK: fnmadd
 ; CHECK-NEXT: blr
+
+; CHECK-VSX-LABEL: test_FNMADD2:
+; CHECK-VSX: xsnmaddmdp
+; CHECK-VSX-NEXT: blr
 }
 
 define double @test_FNMSUB1(double %A, double %B, double %C) {
   %D = fmul double %A, %B   ; <double> [#uses=1]
   %E = fsub double %C, %D   ; <double> [#uses=1]
   ret double %E
 ; CHECK-LABEL: test_FNMSUB1:
 ; CHECK: fnmsub
 ; CHECK-NEXT: blr
+
+; CHECK-VSX-LABEL: test_FNMSUB1:
+; CHECK-VSX: xsnmsubmdp
 }
 
 define double @test_FNMSUB2(double %A, double %B, double %C) {
   %D = fmul double %A, %B   ; <double> [#uses=1]
   %E = fsub double %D, %C   ; <double> [#uses=1]
   %F = fsub double -0.000000e+00, %E    ; <double> [#uses=1]
   ret double %F
 ; CHECK-LABEL: test_FNMSUB2:
 ; CHECK: fnmsub
 ; CHECK-NEXT: blr
+
+; CHECK-VSX-LABEL: test_FNMSUB2:
+; CHECK-VSX: xsnmsubmdp
+; CHECK-VSX-NEXT: blr
 }
 
 define float @test_FNMSUBS(float %A, float %B, float %C) {
   %D = fmul float %A, %B    ; <float> [#uses=1]
   %E = fsub float %D, %C    ; <float> [#uses=1]
   %F = fsub float -0.000000e+00, %E   ; <float> [#uses=1]
   ret float %F
 ; CHECK-LABEL: test_FNMSUBS:
 ; CHECK: fnmsubs
 ; CHECK-NEXT: blr
+
+; CHECK-VSX-LABEL: test_FNMSUBS:
+; CHECK-VSX: fnmsubs
+; CHECK-VSX-NEXT: blr
 }