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

Hal Finkel hfinkel at anl.gov
Thu Oct 2 10:20:25 PDT 2014


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

 -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



More information about the llvm-commits mailing list