[PATCH, PING] Fix common-code ppcf128 component access on little-endian

Hal Finkel hfinkel at anl.gov
Wed Jul 2 21:03:57 PDT 2014


----- Original Message -----
> From: "Ulrich Weigand" <Ulrich.Weigand at de.ibm.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "Alp Toker" <alp at nuanti.com>, "llvm-commits" <llvm-commits at cs.uiuc.edu>
> Sent: Wednesday, July 2, 2014 2:45:56 PM
> Subject: Re: [PATCH,	PING] Fix common-code ppcf128 component access on little-endian
> 
> Hal Finkel <hfinkel at anl.gov> wrote on 30.06.2014 22:55:06:
> 
> > > > > The code itself looks fine, but with the exception of the
> > > > > printing
> > > > > code, is lacking regression tests. Please add regression
> > > > > tests to
> > > > > cover the other changes, and then commit.
> > > >
> > > > Just a drive-by remark, but do you think there could be
> > > > documentation
> > > > value in splitting the check out into a utility function like
> > > > hasBigEndianRepresentation(MVT::SimpleValueType T) instead of
> > > > checking
> > > > MVT::ppcf128 specifically at each point of use?
> > >
> > > Some of the code is ppcf128-specific anyway, but for the other
> > > cases,
> > > I do like the suggestion. I don't like the name, however ;) --
> > > how
> > > about hasBigEndianMemberLayout?
> >
> > Or maybe hasBigEndianPartOrdering? [Simple value types don't really
> > have members].
> 
> Here's an updated version of the patch that uses a helper routine
> hasBigEndianPartOrdering.  It also adds tests to cover the other
> changes.
> 
> In doing so I noticed that I missed a case in ExpandRes_BITCAST,
> and once I added that, it turns out that the original change to
> getCopyToParts is not necessary. (This is because that function
> first uses a BITCAST to convert to an integer type, and then
> operates on the integer type.  The endian swap for ppc_fp128
> takes place during the BITCAST already; no further swap is
> required.)
> 
> Does this look OK?

Much better, thanks! LGTM.

(Alp, thanks for the suggestion, the logic is indeed much clearer this way).

 -Hal

> 
> (See attached file: diff-llvm-le-ppcf128-new)
> 
> Thanks for the review!
> 
> Bye,
> Ulrich
> 
> 

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



More information about the llvm-commits mailing list