[llvm] r220090 - [PowerPC] Change liveness testing in VSX FMA mutation pass

Bill Schmidt wschmidt at linux.vnet.ibm.com
Fri Oct 17 16:35:13 PDT 2014


On Sat, 2014-10-18 at 00:46 +0200, Renato Golin wrote:
> Also, the test seem to be failing on our aarch64 bot:
> 
> http://lab.llvm.org:8011/builders/llvm-aarch64-linux/builds/4699/steps/test-llvm/logs/LLVM%3A%3Afma.ll
> 
> I don't know enough about PPC to infer what's wrong, and I don't have
> access to the board to get you the full dump. Gabor, the owner of that
> bot, is cc'd so if you need, maybe he can get you.

Thanks, Renato -- I've disabled this in r220094 until I can understand
what's going on.  It seems to fail on quite a few builders (perhaps
everything but an actual PowerPC machine) so I'm hopeful I can reproduce
it on an old i686 box.  I'll let you know if I need help running this
down, though.

Thanks!
Bill

> 
> thanks,
> -renato
> 
> On 17 October 2014 23:18, Hal Finkel <hfinkel at anl.gov> wrote:
> > ----- Original Message -----
> >> From: "Bill Schmidt" <wschmidt at linux.vnet.ibm.com>
> >> To: llvm-commits at cs.uiuc.edu
> >> Sent: Friday, October 17, 2014 4:02:44 PM
> >> Subject: [llvm] r220090 - [PowerPC] Change liveness testing in VSX FMA        mutation pass
> >>
> >> Author: wschmidt
> >> Date: Fri Oct 17 16:02:44 2014
> >> New Revision: 220090
> >>
> >> URL: http://llvm.org/viewvc/llvm-project?rev=220090&view=rev
> >> Log:
> >> [PowerPC] Change liveness testing in VSX FMA mutation pass
> >>
> >> With VSX enabled, LLVM crashes when compiling
> >> test/CodeGen/PowerPC/fma.ll.  I traced this to the liveness test
> >> that's revised 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.  At Hal Finkel's request, I converted the liveness test
> >> to an assert restricted to virtual registers.
> >>
> >> I've changed the fma.ll test to have VSX and non-VSX variants so we
> >> can test both kinds of multiply-adds.
> >>
> >> Modified:
> >>     llvm/trunk/lib/Target/PowerPC/PPCInstrInfo.cpp
> >>     llvm/trunk/test/CodeGen/PowerPC/fma.ll
> >>
> >> Modified: llvm/trunk/lib/Target/PowerPC/PPCInstrInfo.cpp
> >> URL:
> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/PowerPC/PPCInstrInfo.cpp?rev=220090&r1=220089&r2=220090&view=diff
> >> ==============================================================================
> >> --- llvm/trunk/lib/Target/PowerPC/PPCInstrInfo.cpp (original)
> >> +++ llvm/trunk/lib/Target/PowerPC/PPCInstrInfo.cpp Fri Oct 17
> >> 16:02:44 2014
> >> @@ -1622,6 +1622,7 @@ protected:
> >>        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;
> >> @@ -1687,16 +1688,26 @@ protected:
> >>          // 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 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.
> >> @@ -1717,10 +1728,11 @@ protected:
> >>          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;
> >> +        // For virtual registers, verify that the addend source
> >> register
> >> +        // is live here (as should have been assured above).
> >> +        if (TargetRegisterInfo::isVirtualRegister(AddendSrcReg))
> >> +          assert(LIS->getInterval(AddendSrcReg).liveAt(FMAIdx) &&
> >> +                 "Addend source register is not available!");
> >
> > Will this even compile in a non-asserts build? Regardless, please make this:
> >
> > assert((!TargetRegisterInfo::isVirtualRegister(AddendSrcReg) || LIS->getInterval(AddendSrcReg).liveAt(FMAIdx)) && "Addend source register is not live!");
> >
> >  -Hal
> >
> >>
> >>          // Transform: (O2 * O3) + O1 -> (O2 * O1) + O3.
> >>
> >>
> >> Modified: llvm/trunk/test/CodeGen/PowerPC/fma.ll
> >> URL:
> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/PowerPC/fma.ll?rev=220090&r1=220089&r2=220090&view=diff
> >> ==============================================================================
> >> --- llvm/trunk/test/CodeGen/PowerPC/fma.ll (original)
> >> +++ llvm/trunk/test/CodeGen/PowerPC/fma.ll Fri Oct 17 16:02:44 2014
> >> @@ -1,4 +1,5 @@
> >> -; 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
> >> @@ -11,6 +12,10 @@ define double @test_FMADD1(double %A, do
> >>  ; 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) {
> >> @@ -20,6 +25,10 @@ define double @test_FMADD2(double %A, do
> >>  ; 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) {
> >> @@ -29,6 +38,10 @@ define double @test_FMSUB1(double %A, do
> >>  ; 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) {
> >> @@ -40,6 +53,10 @@ define double @test_FMSUB2(double %A, do
> >>  ; 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) {
> >> @@ -50,6 +67,10 @@ define double @test_FNMADD1(double %A, d
> >>  ; 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) {
> >> @@ -60,6 +81,10 @@ define double @test_FNMADD2(double %A, d
> >>  ; 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) {
> >> @@ -69,6 +94,9 @@ define double @test_FNMSUB1(double %A, d
> >>  ; 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) {
> >> @@ -79,6 +107,10 @@ define double @test_FNMSUB2(double %A, d
> >>  ; 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) {
> >> @@ -89,4 +121,8 @@ define float @test_FNMSUBS(float %A, flo
> >>  ; CHECK-LABEL: test_FNMSUBS:
> >>  ; CHECK: fnmsubs
> >>  ; CHECK-NEXT: blr
> >> +
> >> +; CHECK-VSX-LABEL: test_FNMSUBS:
> >> +; CHECK-VSX: fnmsubs
> >> +; CHECK-VSX-NEXT: blr
> >>  }
> >>
> >>
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >>
> >
> > --
> > Hal Finkel
> > Assistant Computational Scientist
> > Leadership Computing Facility
> > Argonne National Laboratory
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 





More information about the llvm-commits mailing list