[PATCH] D11438: Fix x86_64 fp128 calling convention

[Chih-Hung Hsieh via llvm-commits]

chh added a comment.

Please continue review other parts while I fix the "goto" statement.

It is possible to further refactor the current floating point legalizing, expanding, customizing modules, but I'm afraid that will be too complex and risky at this moment. Current f128 type for all targets are softened to library calls if it is not "legal" to be "in" registers and each target "customizes" some operations. To fix the x86_64 calling convention problem, we must keep f128 value "in" registers yet still convert most operations to library calls. Hence, a new target configuration of f128 type that is "legal" but softened in most cases.

The changes to f128 "expand" and "custom" operations, although look smaller here, were actually more ad-hoc than my changes to LegalizeFloatTypes.cpp. I usually found missed handling of f128 type from testing libm long double on Android target, and those cases appeared in many different places.


================
Comment at: include/llvm/Target/TargetLowering.h:1919
@@ -1918,2 +1918,3 @@
 
+protected:
   ValueTypeActionImpl ValueTypeActions;
----------------
rnk wrote:
> I think ValueTypeActions should be private to TargetLowering.
It is changed to protected to allow child class X86TargetLowering to do
   ValueTypeActions.setTypeAction(MVT::f128, TypeSoftenFloat);
in Target/X86/X86ISelLowering.cpp.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:54
@@ -53,1 +53,3 @@
 
+  // Some types, e.g. x86_64's f128, want to be legally in registers
+  // but need some operations converted to library calls or integer
----------------
rnk wrote:
> If this is x86_64-specific, why is it in LegalizeFloatTypes.cpp?
I thought at the beginning that LegalizeFloatTypes should be target independent.
However I found many special case in this file for MVT::ppcf128.
Looking at how ppcf128 is handled now, I think it is best for me to follow the structure in similar places.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:77
@@ +76,3 @@
+      case ISD::ConstantFP:
+        goto SaveResult;
+    }
----------------
rnk wrote:
> I'm not really sure what's up with this code, but adding goto doesn't feel particularly good.
There is not much reason to use "goto" now. I will change it in the next diff.
It was simpler to share more code at SaveResult point before return.

I found that SoftenFloatResult serves two purposes:
(1) generates calls to library functions,
(2) converts float to integer values and operations.
Not all fp types need (2) so I added KeepFloat flag to skip (2) for some cases.



http://reviews.llvm.org/D11438