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

[Bill Seurer]

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
>>>>> 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
>>>
>>> I ran into a wrinkle with using the FastISel-specific isTypeLegal.
>>>   Its
>>> second parameter (an MVT) is a reference and it actually updates
>>> the
>>> value of the passed in argument via the reference.  This is
>>> different
>>> behavior than what TLI.isLegal does and most likely not wanted.
>>
>> Just taking a step backward here, but it looks like working these
>> checks
>> 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
>> to
>> 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
>> by
>> default early in the 3.6 release cycle so we can get plenty of
>> burn-in
>> 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).
>
>   -Hal

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 
separate patch.
-- 

-Bill Seurer