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

[Alp Toker]

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?

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