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

[Hal Finkel]

----- Original Message -----
> From: "Alp Toker" <alp at nuanti.com>
> To: "Hal Finkel" <hfinkel at anl.gov>, "Ulrich Weigand" <Ulrich.Weigand at de.ibm.com>
> Cc: "llvm-commits" <llvm-commits at cs.uiuc.edu>
> Sent: Monday, June 30, 2014 3:46:45 PM
> Subject: Re: [PATCH,	PING] Fix common-code ppcf128 component access on little-endian
> 
> 
> On 30/06/2014 23:01, Hal Finkel wrote:
> > ----- Original Message -----
> >> From: "Ulrich Weigand" <Ulrich.Weigand at de.ibm.com>
> >> To: "llvm-commits" <llvm-commits at cs.uiuc.edu>
> >> Sent: Monday, June 30, 2014 2:14:53 PM
> >> Subject: [PATCH,	PING] Fix common-code ppcf128 component access on
> >> little-endian
> >>
> >>
> >>
> >> Hello,
> >>
> >> the PowerPC 128-bit long double data type (ppcf128 in LLVM) is in
> >> fact a
> >> pair of two doubles, where one is considered the "high" or
> >> more-significant
> >> part, and the other is considered the "low" or less-significant
> >> part.
> >>   When
> >> a ppcf128 value is stored in memory or a register pair, the high
> >> part
> >> always comes first, i.e. at the lower memory address or in the
> >> lower-numbered register, and the low part always comes second.
> >>  This
> >> is
> >> true both on big-endian and little-endian PowerPC systems.
> >>  (Similar
> >> to how
> >> with a complex number, the real part always comes first and the
> >> imaginary
> >> part second, no matter the byte order of the system.)
> >>
> >> This is currently implemented incorrectly for little-endian
> >> systems
> >> in
> >> LLVM.   There are three issues I've noticed in my tests on
> >> ppc64le:
> >>
> >> - When printing an immediate ppcf128 constant to assembler output,
> >> current
> >> code in emitGlobalConstantFP does indeed correctly emit the high
> >> part
> >> first
> >> on big-endian systems, but emits the *low* part first on
> >> little-endian
> >> systems.
> >> - When lowering a ppcf128 type to a pair of f64 types in
> >> SelectionDAG
> >> (which is used e.g. when generating code to load an argument into
> >> a
> >> register pair), low and high part are inverted on little-endian
> >> systems.
> >> - In a related issue, because lowering ppcf128 into a pair of f64
> >> must
> >> operate differently from lowering an int128 into a pair of i64,
> >> bitcasts
> >> between ppcf128 and int128 must not be optimized away by the DAG
> >> combiner.
> >>
> >> The attached patch fixes these issues; this makes all native
> >> testing
> >> on
> >> powerpc64le pass (including full test-suite and bootstrap), when
> >> my
> >> not-yet-published ELFv2 patches are also applied.
> >>
> >> I'd appreciate a review; is this OK to commit?
> > 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?

 -Hal

> 
> Alp.
> 
> >
> > Thanks again,
> > Hal
> >
> >> (See attached file: diff-llvm-le-ppcf128)
> >>
> >>
> >> Mit freundlichen Gruessen / Best Regards
> >>
> >> Ulrich Weigand
> >>
> >> --
> >>    Dr. Ulrich Weigand | Phone: +49-7031/16-3727
> >>    STSM, GNU/Linux compilers and toolchain
> >>    IBM Deutschland Research & Development GmbH
> >>    Vorsitzende des Aufsichtsrats: Martina Koederitz |
> >>    Geschäftsführung: Dirk
> >> Wittkopp
> >>    Sitz der Gesellschaft: Böblingen | Registergericht: Amtsgericht
> >> Stuttgart, HRB 243294
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >>
> 
> --
> http://www.nuanti.com
> the browser experts
> 
> 

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