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

[Eric Christopher]

On Thu, Oct 2, 2014 at 10:20 AM, Hal Finkel <hfinkel at anl.gov> wrote:
> ----- Original Message -----
>> From: "Eric Christopher" <echristo at gmail.com>
>> To: "Hal Finkel" <hfinkel at anl.gov>
>> Cc: "Bill Schmidt" <wschmidt at linux.vnet.ibm.com>, reviews+D5362+public+ecca475fdb0fc511 at reviews.llvm.org,
>> llvm-commits at cs.uiuc.edu, "Bill Seurer" <seurer at linux.vnet.ibm.com>
>> Sent: Thursday, October 2, 2014 11:38:08 AM
>> Subject: Re: [PATCH] [PATCH] Test case and FastISel fixes with FeatureVSX enabled
>>
>> 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.
>
> I'd rather not lose the coverage entirely, but that's an option. In any case, from Bill S.'s other e-mail, it sounds like he has a solution using isTypeLegal after all.
>

OK. Sounds good.

-eric

>  -Hal
>
>>
>> -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
>>
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory