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

[Hal Finkel]

----- Original Message -----
> From: "Eric Christopher" <echristo at gmail.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: seurer at linux.vnet.ibm.com, "Daniel Sanders" <daniel.sanders at imgtec.com>, "reed kotler" <rkotler at mips.com>, "Bill
> Schmidt" <wschmidt at linux.vnet.ibm.com>, llvm-commits at cs.uiuc.edu,
> reviews+D5362+public+ecca475fdb0fc511 at reviews.llvm.org
> Sent: Friday, September 19, 2014 4:07:06 PM
> Subject: Re: [PATCH] [PATCH] Test case and FastISel fixes with FeatureVSX enabled
> 
> On Fri, Sep 19, 2014 at 1:32 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> > ----- Original Message -----
> >> From: "Eric Christopher" <echristo at gmail.com>
> >> To: "Hal Finkel" <hfinkel at anl.gov>
> >> Cc: seurer at linux.vnet.ibm.com, "Daniel Sanders"
> >> <daniel.sanders at imgtec.com>, "reed kotler" <rkotler at mips.com>,
> >> "Bill
> >> Schmidt" <wschmidt at linux.vnet.ibm.com>, llvm-commits at cs.uiuc.edu,
> >> reviews+D5362+public+ecca475fdb0fc511 at reviews.llvm.org
> >> Sent: Friday, September 19, 2014 1:28:02 PM
> >> Subject: Re: [PATCH] [PATCH] Test case and FastISel fixes with
> >> FeatureVSX enabled
> >>
> >> On Fri, Sep 19, 2014 at 11:19 AM, Hal Finkel <hfinkel at anl.gov>
> >> wrote:
> >> > ----- Original Message -----
> >> >> From: "Eric Christopher" <echristo at gmail.com>
> >> >> To: reviews+D5362+public+ecca475fdb0fc511 at reviews.llvm.org
> >> >> Cc: seurer at linux.vnet.ibm.com, "Daniel Sanders"
> >> >> <daniel.sanders at imgtec.com>, "reed kotler" <rkotler at mips.com>,
> >> >> "Bill
> >> >> Schmidt" <wschmidt at linux.vnet.ibm.com>, "Hal Finkel"
> >> >> <hfinkel at anl.gov>, llvm-commits at cs.uiuc.edu
> >> >> Sent: Friday, September 19, 2014 11:52:23 AM
> >> >> Subject: Re: [PATCH] [PATCH] Test case and FastISel fixes with
> >> >> FeatureVSX enabled
> >> >>
> >> >> On Fri, Sep 19, 2014 at 5:17 AM, hfinkel at anl.gov
> >> >> <hfinkel at anl.gov>
> >> >> wrote:
> >> >> > Thanks for working on this!
> >> >> >
> >> >> > Regarding approach, instead of disabling parts of FastISel
> >> >> > when
> >> >> > VSX
> >> >> > is enabled, why don't we disable VSX when FastISel would be
> >> >> > used?
> >> >> > For example, what if in PPCSubtarget::initSubtargetFeatures
> >> >> > we
> >> >> > added something like this after the call to
> >> >> > ParseSubtargetFeatures:
> >> >> >
> >> >> >   if (OptLevel == CodeGenOpt::None) or even better if
> >> >> >   possible
> >> >> >   (TM.Options.EnableFastISel)
> >> >> >     HasVSX = false;
> >> >> >
> >> >> > (we already do something similar for little-Endian mode
> >> >> > currently).
> >> >> > Then we just need to add an assert in the FastISel
> >> >> > implementation
> >> >> > that VSX is not enabled.
> >> >> >
> >> >>
> >> >> What about builtins? If you don't enable the subtarget feature
> >> >> you
> >> >> won't be able to select any code that uses VSX in fast isel
> >> >> mode -
> >> >> even if you could with the selection dag.
> >> >
> >> > We don't have any currently ;) -- but this is a good point. Long
> >> > term, at least, this definitely needs to be fixed properly.
> >> >
> >> > Another options is just to refactor the check into a separate
> >> > function so that we're not repeating the comment explaining what
> >> > is going on in many places. That's also fine with me.
> >> >
> >>
> >> Right, that's what I meant by "handled in the isTypeLegal
> >> machinery"
> >> in my original feedback.
> >
> > Ah, right. That also makes sense. We could return false from
> > isTypeLegal when VSX is enabled for f64, etc.
> >
> 
> Could, might not, but could. Limiting the regclasses for fast-isel
> hurts performance quite a bit - you'd start failing to select again
> and fall back to the DAG. You probably only want to do that if it
> really is illegal.

Which is why I recommended just disabling VSX when using FastISel (this won't hurt performance as much, and this is supposed to be a temporary work-around anyway).

Another solution might be to insert a register-class copy back to the scalar FP registers when we get a VSX register. Really, the solution here is to teach FastISel about the relevant basic VSX instructions, but I think that getting VSX enabled by default when optimizing is worth a work-around for -O0 in the mean time.

 -Hal

> 
> -eric
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory