[PATCH] [PATCH] Test case and FastISel fixes with FeatureVSX enabled
Bill Seurer
seurer at linux.vnet.ibm.com
Thu Sep 25 08:21:11 PDT 2014
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
More information about the llvm-commits
mailing list