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

[Eric Christopher]

On Thu, Oct 2, 2014 at 6:21 AM, Hal Finkel <hfinkel at anl.gov> 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).
>

Honestly, if you're just going to do a temporary fix you can just ask
the subtarget if vsx is enabled and just not turn on fast isel at all.

-eric

>  -Hal
>
>>
>> Thanks,
>> Bill
>>
>> >
>> > While experimenting with this to see what happened anyway I ran
>> > into
>> > something else:
>> >
>> > bool SelectionDAGISel::runOnMachineFunction(MachineFunction &mf) {
>> >    // Do some sanity-checking on the command-line options.
>> >    assert((!EnableFastISelVerbose || TM.Options.EnableFastISel) &&
>> >           "-fast-isel-verbose requires -fast-isel");
>> >    assert((!EnableFastISelAbort || TM.Options.EnableFastISel) &&
>> >           "-fast-isel-abort requires -fast-isel");
>> >
>> > The second assert states that -fast-isel-abort requires -fast-isel
>> > but
>> > there are many PPC (and maybe other target) test cases that use
>> > only
>> > -fast-isel-abort as an option.  Is this really a requirement?  If
>> > so,
>> > why doesn't -fast-isel-abort either check that -fast-isel was
>> > specified
>> > or even just set it itself?
>>
>>
>>
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory