[PATCH] D11438: Part 2 to fix x86_64 fp128 calling convention.

[Xinliang David Li via llvm-commits]

On Fri, Dec 11, 2015 at 3:38 PM, Chih-Hung Hsieh <chh at google.com> wrote:
> chh added inline comments.
>
> ================
> Comment at: test/CodeGen/X86/fp128-cast.ll:136
> @@ +135,3 @@
> +define void @TestUIToFPU32_F128() #2 {
> +entry:
> +  %0 = load i32, i32* @vu32, align 4
> ----------------
> davidxl wrote:
>> Why do we care what transformations have been done to get the IR? The IR code should by  itself readable -- so while the C example is useful, I still prefer the naming in IR simplified.
> The comment now seems to be showing at wrong place in this code diff.
> The biggest confusing name is u.sroa.0.4.extract.shift  in TestBits128.
> So I will shorten long names in this function for now.
>
> The C code is important for anyone in the future to test, if not having time to rebuild and test the whole AOSP with libm.
> Any simplification of the IR might work, but clang could generate different bit operators for those long double union types and trigger problems with any ad-hoc f128 optimization.
>
>
>
> ================
> Comment at: test/CodeGen/X86/fp128-compare.ll:34
> @@ +33,3 @@
> +  ret i32 %conv
> +; CHECK-LABEL: TestComp128LT:
> +; CHECK:       callq __lttf2
> ----------------
> davidxl wrote:
>> ok. But it is possible with test + sets, right?  may be adding a comment so that people know how to fix the test if it breaks in the future?
> Sure, added comment.
> If it is broken in the future, maybe it would be easier to continue using this trick. :-)
>
>
> ================
> Comment at: test/CodeGen/X86/fp128-i128.ll:7
> @@ +6,3 @@
> +; union IEEEl2bits {
> +;   long double e;
> +;   struct {
> ----------------
> davidxl wrote:
>> __float128
> Unfortunately, clang does not accept __float128 keyword, although it can emit f128 for llvm.
>
>
> ================
> Comment at: test/CodeGen/X86/fp128-i128.ll:23
> @@ +22,3 @@
> +; void foo(long double x);
> +; void TestUnionLD1(long double s, unsigned long n) {
> +;      union IEEEl2bits u;
> ----------------
> davidxl wrote:
>> long double --> __float128?
> no __float128 in clang.
>
>
> ================
> Comment at: test/CodeGen/X86/fp128-i128.ll:44
> @@ +43,3 @@
> +; CHECK:       movaps %xmm0, -24(%rsp)
> +; CHECK-NEXT:  movq -24(%rsp), %rax
> +; CHECK-NEXT:  movabsq $281474976710655, %rcx
> ----------------
> davidxl wrote:
>> The pattern checked is pretty long -- I worry it may break in the future. Is it possible to relax it some how?
> I tried to reduce the C code, but any reduction won't trigger the complicated IL that reached a point that my partial fix core dumped. Maybe we can take out a few CHECK-NEXT requirements.

Where does it core-dump? If it is in new code, it needs to be fixed :)
Otherwise file a bug.

>
> On the other hard, I was terrified by so many ad-hoc optimizations of floating points for the usage patterns in libm.
> I guess llvm tried to match or do better then gcc and libm tried to use every possible bit operations.
> So maybe it is better for Android or anyone depends on f128 type to have more check rules here.
> Whoever changes float optimization in the future has better fully understand and update these tests.

Maybe these complicated/fragile case do not belong here -- but
somewhere downstream (libm).

David


>
>
>
>
> http://reviews.llvm.org/D11438
>
>
>