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

[Hal Finkel]

----- Original Message -----
> From: "Hal Finkel" <hfinkel at anl.gov>
> To: "Alp Toker" <alp at nuanti.com>
> Cc: "llvm-commits" <llvm-commits at cs.uiuc.edu>, "Ulrich Weigand" <Ulrich.Weigand at de.ibm.com>
> Sent: Monday, June 30, 2014 3:52:51 PM
> Subject: Re: [PATCH,	PING] Fix common-code ppcf128 component access on little-endian
> 
> ----- 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?

Or maybe hasBigEndianPartOrdering? [Simple value types don't really have members].

 -Hal

> 
>  -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
> 

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