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

[Alp Toker]

On 30/06/2014 23:55, Hal Finkel wrote:
> ----- 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].

Take your pick :-)

Alp.

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

-- 
http://www.nuanti.com
the browser experts