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

[Bill Schmidt]

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.

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?