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

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: test/CodeGen/X86/fp128-calling-conv.ll:5
@@ +4,3 @@
+; double myD = 1.0;
+ at myD = global double 1.000000e+00, align 8
+
----------------
Is this variable used?

================
Comment at: test/CodeGen/X86/fp128-calling-conv.ll:8
@@ +7,3 @@
+; long double myFP80 = 1.0L;  // x86_64-linux-gnu
+ at myFP80 = global x86_fp80 0xK3FFF8000000000000000, align 16
+
----------------
Is it used?

================
Comment at: test/CodeGen/X86/fp128-calling-conv.ll:11
@@ +10,3 @@
+; long double myFP128 = 1.0L;  // x86_64-linux-android
+ at myFP128 = global fp128 0xL00000000000000003FFF000000000000, align 16
+
----------------
Are the two parts swapped? GCC seems to generates: 3FFF0000000000000000000000000000 

================
Comment at: test/CodeGen/X86/fp128-cast.ll:12
@@ +11,3 @@
+
+define void @TestCastF32_F128() {
+entry:
----------------
TestFPExtF32_F128

================
Comment at: test/CodeGen/X86/fp128-cast.ll:25
@@ +24,3 @@
+
+define void @TestCastF64_F128() {
+entry:
----------------
TestFPExt ..

================
Comment at: test/CodeGen/X86/fp128-cast.ll:38
@@ +37,3 @@
+
+define void @TestCastF128_I32() {
+entry:
----------------
Missing a test for conversion to unsigned I32

================
Comment at: test/CodeGen/X86/fp128-cast.ll:126
@@ +125,3 @@
+; CHECK-NEXT:  callq __gttf2
+; CHECK-NEXT:  testl %eax, %eax
+; CHECK-NEXT:  setg %al
----------------
might be better to relax the check a little  : testq %rax, %rax should be fine too.

================
Comment at: test/CodeGen/X86/fp128-cast.ll:135
@@ +134,3 @@
+  %0 = bitcast fp128 %mul to i128
+  %u.sroa.0.4.extract.shift = lshr i128 %0, 32
+  %or5 = or i128 %u.sroa.0.4.extract.shift, %0
----------------
Can you simplify the variable names ?

================
Comment at: test/CodeGen/X86/fp128-cast.ll:166
@@ +165,3 @@
+; CHECK-NEXT:  movq %rsi, -24(%rsp)
+; CHECK-NEXT:  adcq $0, %rdi
+; CHECK-NEXT:  movq %rdi, -16(%rsp)
----------------
There is no guarantee adcq will be after movq ...

================
Comment at: test/CodeGen/X86/fp128-compare.ll:23
@@ +22,3 @@
+; CHECK:       testl %eax, %eax
+; CHECK:       retq
+}
----------------
missing check of 'set<cc>'

================
Comment at: test/CodeGen/X86/fp128-compare.ll:33
@@ +32,3 @@
+; CHECK:       callq __lttf2
+; CHECK-NEXT:  shrl $31, %eax
+; CHECK:       retq
----------------
is this correct?

================
Comment at: test/CodeGen/X86/fp128-compare.ll:45
@@ +44,3 @@
+; CHECK-NEXT:  testl %eax, %eax
+; CHECK:       retq
+}
----------------
Missing check

================
Comment at: test/CodeGen/X86/fp128-i128.ll:4
@@ +3,3 @@
+
+; Check some i128 instruction patterns triggered by fp128.
+
----------------
Better comment?

================
Comment at: test/CodeGen/X86/fp128-i128.ll:92
@@ +91,3 @@
+if.end:                                           ; preds = %if.then, %entry
+  %u.sroa.0.0 = phi i128 [ %bf.set, %if.then ], [ %0, %entry ]
+  %2 = bitcast i128 %u.sroa.0.0 to fp128
----------------
var names can be cleaned up to be shorter.

================
Comment at: test/CodeGen/X86/fp128-i128.ll:125
@@ +124,3 @@
+define { i64, i64 } @TestShift128(i64 %x.coerce0, i64 %x.coerce1) #0 {
+entry:
+  %.fca.1.insert = insertvalue { i64, i64 } { i64 0, i64 undef }, i64 %x.coerce0, 1
----------------
seems irrelevant.

================
Comment at: test/CodeGen/X86/fp128-i128.ll:137
@@ +136,3 @@
+
+define void @TestShift128_2() #2 {
+entry:
----------------
Seems irrelevant.

================
Comment at: test/CodeGen/X86/fp128-i128.ll:205
@@ +204,3 @@
+; Test more complicated logical operations generated from copysignl.
+define void @TestCopySign({ fp128, fp128 }* noalias nocapture sret %agg.result, { fp128, fp128 }* byval nocapture readonly align 16 %z) #0 {
+entry:
----------------
Can this test case be simplified more?


http://reviews.llvm.org/D11438