[PATCH] D67128: [X86] Move x86_64 fp128 conversion to libcalls from type legalization to DAG legalization

[Chih-Hung Hsieh via Phabricator via llvm-commits]

chh added a comment.

I haven't read all code details yet; just some quick questions and comments.

Are the 3 test cases the only ones changed?
I know that we don't have very good coverage.
The ultimate test will be compatibility with Android x86_64 target,
which has some unique ABI not the same as AArch64 and normal x86_64.

Among those 3 changed unit tests, 2 seemed getting longer output code.
Could they be improved to be as good as before?

I'm surprised that no change is needed to LegalizeFloatTypes.cpp.
On the other hand, please keep this change as small as possible.
In the worst case, if we find any regression, we will revert it in the Android release.



================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:626
 
-  // Long double always uses X87, except f128 in SSE.
+  // Long double always uses X87.
   if (UseX87) {
----------------
This comment is incorrect now. f128 still should be stored in SSE when available.



================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:682-693
+    if (isTypeLegal(MVT::f32)) {
+      setOperationAction(ISD::FP_ROUND, MVT::f32, Custom);
+      setOperationAction(ISD::STRICT_FP_ROUND, MVT::f32, Custom);
+    }
+    if (isTypeLegal(MVT::f64)) {
+      setOperationAction(ISD::FP_ROUND, MVT::f64, Custom);
+      setOperationAction(ISD::STRICT_FP_ROUND, MVT::f64, Custom);
----------------
Are the changes to f32, f64, f80 dependent to this f128 change?
If they are, please add some comment.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67128/new/

https://reviews.llvm.org/D67128