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

Hal Finkel hfinkel at anl.gov
Thu Oct 2 10:18:45 PDT 2014


----- Original Message -----
> From: "Bill Seurer" <seurer at linux.vnet.ibm.com>
> To: "Hal Finkel" <hfinkel at anl.gov>, "Bill Schmidt" <wschmidt at linux.vnet.ibm.com>
> 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, October 2, 2014 10:03:56 AM
> Subject: Re: [PATCH] [PATCH] Test case and FastISel fixes with FeatureVSX enabled
> 
> On 10/2/2014 8:21 AM, Hal Finkel wrote:
> > ----- 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
> 
> I actually have this working just fine.  I added a new isTypeLegal in
> FastISel (the base class) that has the same parameter list as the
> TLI.isTypeLegal one (just one parm, an EVT).  By default in the base
> class the new one just calls TLI.isTypeLegal so nothing will change
> for
> existing code of other targets.  I override the new function in
> PPCFastISel and do what we want there.
> 
> Some code is common between the old isTypeLegal in PPCFastISel that
> takes two parms and uses the second reference parm to change the
> value
> of the MVT argument and the new one parm isTypeLegal.  But because of
> the reference parm thing I don't see a clean way of having one use
> the
> other.  I could add another new function that extracts the common
> code
> between them I suppose.

That would be good if possible.

> 
> Also, I am wondering if the test case changes should have been done
> as a
> separate patch.

What do you mean? You can certainly separate functionality-changing from non-functionality-changing patches.

 -Hal

> --
> 
> -Bill Seurer
> 
> 

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



More information about the llvm-commits mailing list