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

[Eric Christopher]

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.

-eric