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

[Bill Seurer]

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.

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