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

[Eric Christopher]

Here's how this should probably go:

a) rewrite patch to use isTypeLegal and turn off everything since that
should be a simple refactor of the existing one,
b) add support for operations using register copy as you mention while
moving the code from isLegalType to isLoadStoreLegalType (or whatever
it is, I can't recall), and then
c) fix the load store code, and finally
d) support vsx operations in fast isel.

Any of those past a) can probably be folded into d) if you just go
work on that next.

Thanks!

-eric

On Fri, Sep 19, 2014 at 2:23 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 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