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

[Hal Finkel]

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

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