[PATCH] [PATCH] Test case and FastISel fixes with FeatureVSX enabled

[Hal Finkel]

----- Original Message -----
> From: "Bill Seurer" <seurer at linux.vnet.ibm.com>
> To: "Bill Schmidt" <wschmidt at linux.vnet.ibm.com>, "Hal Finkel" <hfinkel at anl.gov>
> Cc: reviews+D5362+public+ecca475fdb0fc511 at reviews.llvm.org, "Daniel Sanders" <daniel.sanders at imgtec.com>, "reed
> kotler" <rkotler at mips.com>, llvm-commits at cs.uiuc.edu, "Eric Christopher" <echristo at gmail.com>
> Sent: Thursday, September 25, 2014 10:21:11 AM
> Subject: Re: [PATCH] [PATCH] Test case and FastISel fixes with FeatureVSX enabled
> 
> On 9/22/2014 2:03 PM, Bill Schmidt wrote:
> > On Mon, 2014-09-22 at 13:57 -0500, Hal Finkel wrote:
> >> ----- Original Message -----
> >>> From: "Bill Schmidt" <wschmidt at linux.vnet.ibm.com>
> >>> To: "Eric Christopher" <echristo at gmail.com>
> >>> Cc: reviews+D5362+public+ecca475fdb0fc511 at reviews.llvm.org,
> >>> seurer at linux.vnet.ibm.com, "Daniel Sanders"
> >>> <daniel.sanders at imgtec.com>, "reed kotler" <rkotler at mips.com>,
> >>> "Hal Finkel" <hfinkel at anl.gov>,
> >>> llvm-commits at cs.uiuc.edu
> >>> Sent: Monday, September 22, 2014 1:50:43 PM
> >>> Subject: Re: [PATCH] [PATCH] Test case and FastISel fixes with
> >>> FeatureVSX enabled
> >>>
> >>> On Mon, 2014-09-22 at 10:05 -0700, Eric Christopher wrote:
> >>>>>> How about changing getRegForValue to return 0 if it ends up
> >>>>>> with
> >>>>>> a VSX register.  All the uses of getRegForValue already check
> >>>>>> for a 0 return value.  That moves the code to just one spot
> >>>>>> and
> >>>>>> there are already multiple checks there for things that
> >>>>>> FastISel can't handle.
> >>>>>
> >>>>> Hi Bill,
> >>>>>
> >>>>> I believe this is generally what Eric is suggesting, but
> >>>>> FastISel::getRegForValue() is in the common code, so we have to
> >>>>> change
> >>>>> it in one of the backend-specific calls.  There is a call to
> >>>>> TLI.isTypeLegal(VT) in FastISel::getRegForValue() that will
> >>>>> call
> >>>>> into
> >>>>> PPCFastISel.isTypeLegal().  So you can add a check there for
> >>>>> the
> >>>>> VSX
> >>>>> register classes and return false for now, and that will take
> >>>>> care of it
> >>>>> everywhere.
> >>>>>
> >>>>
> >>>> Not the register classes, but the types that would lead to those
> >>>> classes. (f128 probably, etc)
> >>>
> >>> Right, I misspoke (with my phalanges).  I think you should be
> >>> able to
> >>> just return false when VSX is enabled and VT.isVector() is true.
> >>
> >> And for f64 too. It's that whole 'scalar' part of your
> >> 'vector-scalar extensions' ;)
> >
> > Eh, right. :)  Hey, I've been on vacation, I need a couple days to
> > get
> > my head back in the game...
> 
> There is a problem in just trying to use PPCFastISel.isTypeLegal().
> There are two places in FastISel::getRegForValue where a register is
> actually gotten. The first place involves the above mentioned call to
> TLI.isTypeLegal(VT) BUT this does not get to PPCFastISel.isTypeLegal
> because it goes into TargetLoweringBase::isTypeLegal instead.

Okay, so generally speaking, there are about 8 places in lib/CodeGen/SelectionDAG/FastISel.cpp that call TLI.isTypeLegal (and I see no calls to the FastISel-specific isTypeLegal). Perhaps this should be changed?

 -Hal

> 
> Changing that (or the PPC version thereof) is probably not a good
> idea.
> 
> 
> The gory details follow...
> 
> 
> This is some code (excerpted from fast-isel-load-store.ll) that shows
> the above behavior.
> 
> ; RUN: llc < %s -O0 -verify-machineinstrs -fast-isel-abort
> -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr7 | FileCheck %s
> --check-prefix=ELF64
> 
> @f = global double 3.5, align 8
> 
> define double @t6() nounwind uwtable ssp {
> ; ELF64: t6
>    %1 = load double* @f, align 8
> ; ELF64: lfd
>    %2 = fadd double %1, 1.0
> ; ELF64: fadd
>    ret double %2
> }
> 
> 
> If you run it with -mattr=+vsx it will die.  I added an assert in
> PPCFastISel::SelectRet when I was trying to figure out where in a
> some
> test cases it was still getting VSX registers from getRegForValue
> despite my changes to PPCFastISel.isTypeLegal:
> 
>        unsigned Reg = getRegForValue(RV);  // line 1599
> 
>        if (Reg == 0)
>          return false;
>        assert((MRI.getRegClass(Reg)->getID() != PPC::VSFRCRegClassID)
>        &&
> "!@! WDS: got a VSX register");
> 
> and when I run it the PPC isTypeLegal is never called but the
> assertion
> does go off...
> 
> (gdb) break PPCFastISel.cpp:258   ...this is PPC isTypeLegal...
> (gdb) run
> llc:
> /home/seurer/llvm/llvm-test/lib/Target/PowerPC/PPCFastISel.cpp:1604:
> bool {anonymous}::PPCFastISel::SelectRet(const
>   llvm::Instruction*): Assertion `(MRI.getRegClass(Reg)->getID() !=
> PPC::VSFRCRegClassID) && "!@! WDS: got a VSX register
> "' failed.
> 
> If I step through what is happening...
> 
> (gdb) break PPCFastISel.cpp:1599
> (gdb) run
> 1599          unsigned Reg = getRegForValue(RV);
> (gdb) s
> 167       EVT RealVT = TLI.getValueType(V->getType(),
> /*AllowUnknown=*/true);
> ...stepping a bit...
> 176       if (!TLI.isTypeLegal(VT)) {
> (gdb) s
> ...stepping a bit...
> llvm::TargetLoweringBase::isTypeLegal (this=0x12c903a0, VT=...)
>      at
>      /home/seurer/llvm/llvm-test/include/llvm/Target/TargetLowering.h:355
> 355         assert(!VT.isSimple() ||
> (gdb) n
> 356                (unsigned)VT.getSimpleVT().SimpleTy <
> array_lengthof(RegClassForVT));
> (gdb) n
> 357         return VT.isSimple() &&
> RegClassForVT[VT.getSimpleVT().SimpleTy] != nullptr;
> (gdb) print VT.isSimple()
> $1 = true
> (gdb) print RegClassForVT[VT.getSimpleVT().SimpleTy]
> $2 = (const llvm::TargetRegisterClass *) 0x12c4cf18
> <llvm::PPC::VSFRCRegClass>
> 
> So this isTypeLegal is going to return true.  Eventually it gets back
> to
> the following which is what actually allocates the VSX register:
> 
> llvm::FastISel::getRegForValue (this=0x12cbc610, V=0x12c8f1c0)
>      at
> /home/seurer/llvm/llvm-test/lib/CodeGen/SelectionDAG/FastISel.cpp:194
> 194         return FuncInfo.InitializeRegForValue(V);
> (gdb) finish
> 1599          unsigned Reg = getRegForValue(RV);
> (gdb) print/x $4
> $5 = 0x80000000
> (gdb) finish
> llc:
> /home/seurer/llvm/llvm-test/lib/Target/PowerPC/PPCFastISel.cpp:1604:
> bool {anonymous}::PPCFastISel::SelectRet(const
>   llvm::Instruction*): Assertion `(MRI.getRegClass(Reg)->getID() !=
> PPC::VSFRCRegClassID) && "!@! WDS: got a VSX register
> "' failed.
> 
> So it *is* calling isTypeLegal  but just not the FastISel
> PPC-specific
> one.  And thus the VSX register allocation slips through.
> 
> --
> 
> -Bill Seurer
> 
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory