[PATCH] [PATCH] Test case and FastISel fixes with FeatureVSX enabled
seurer at linux.vnet.ibm.com
Thu Oct 2 08:03:56 PDT 2014
On 10/2/2014 8:21 AM, Hal Finkel wrote:
> ----- Original Message -----
>> From: "Bill Schmidt" <wschmidt at linux.vnet.ibm.com>
>> To: "Bill Seurer" <seurer at linux.vnet.ibm.com>
>> Cc: "Hal Finkel" <hfinkel at anl.gov>, 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, October 2, 2014 8:18:12 AM
>> Subject: Re: [PATCH] [PATCH] Test case and FastISel fixes with FeatureVSX enabled
>> On Tue, 2014-09-30 at 16:55 -0500, Bill Seurer wrote:
>>> On 9/25/2014 10:33 AM, Hal Finkel wrote:
>>>> ----- 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
>>>>> There is a problem in just trying to use
>>>>> 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
>>>>> 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?
>>> I ran into a wrinkle with using the FastISel-specific isTypeLegal.
>>> second parameter (an MVT) is a reference and it actually updates
>>> value of the passed in argument via the reference. This is
>>> behavior than what TLI.isLegal does and most likely not wanted.
>> Just taking a step backward here, but it looks like working these
>> into the isTypeLegal machinery is not going anywhere. Eric and Hal,
>> since this is a temporary fix anyway, what do you think of going back
>> Bill's original approach? The code duplication isn't pretty, but I'm
>> not seeing a clean alternative. It would be nice to get VSX enabled
>> default early in the 3.6 release cycle so we can get plenty of
>> on it.
> Yes, let's do that: his original approach (as modified by you so that the change is only in the one function), and add a FIXME that putting this in isTypeLegal is the "right" way to do this (with an explanation of the problems).
I actually have this working just fine. I added a new isTypeLegal in
FastISel (the base class) that has the same parameter list as the
TLI.isTypeLegal one (just one parm, an EVT). By default in the base
class the new one just calls TLI.isTypeLegal so nothing will change for
existing code of other targets. I override the new function in
PPCFastISel and do what we want there.
Some code is common between the old isTypeLegal in PPCFastISel that
takes two parms and uses the second reference parm to change the value
of the MVT argument and the new one parm isTypeLegal. But because of
the reference parm thing I don't see a clean way of having one use the
other. I could add another new function that extracts the common code
between them I suppose.
Also, I am wondering if the test case changes should have been done as a
More information about the llvm-commits