[PATCH] D11438: Fix x86_64 fp128 calling convention

[Quentin Colombet via llvm-commits]

qcolombet added a comment.

Hi Chih-Hung,

I believe the current patch can be split in several smaller patches. For instance, improving the AsmPrinter and changing the ABI seem pretty orthogonal to not have to come in one patch.

Now, as a high level comment, this patch seems quite involved for something that sounds fairly common from the legalizer point of view. I.e., we have a legal type, fp128, with illegal instructions (soften float). Have you checked how we deal with those cases usually? If so, could you explain what does and does not work?
I have not spent a lot of time on thinking or reviewing the patch, so I admit I can tell nonsense, but the fell I have on the patch is that we are doing it wrong.

Regarding the ABI changes, the patch obviously breaks the existing code that have been generated by clang for those types. I would like to have a sense of how risky are that changes. Could you list the targets (OS) that use the variants you are changing?

Thanks,
-Quentin


================
Comment at: include/llvm/Target/TargetLowering.h:1907
@@ -1906,2 +1906,3 @@
+protected:
   ValueTypeActionImpl ValueTypeActions;
 
----------------
That change makes me shiver.
We probably do something against the design if we need to do that.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8592
@@ +8591,3 @@
+  // instruction selection cannot handle FCOPYSIGN on
+  // SSE registers yet.
+  EVT N1VT = N1->getValueType(0);
----------------
Based on the comment, this seems like a hack that impact all targets to fix a x86_64 backend limitation.
The bottom line is that I am guessing we should push that change.

================
Comment at: lib/CodeGen/SelectionDAG/InstrEmitter.cpp:147
@@ -143,1 +146,3 @@
+              // Until that behavior of FR128 is changed,
+              // we need to handle such case later before using UseRC.
               // If multiple uses expect disjoint register classes, we emit
----------------
By definition the common sub class of A and B must be able to hold both the types of A and B, since the related registers are both in A and in B (i.e., could be access by A or B register names).
The bottom line is that comment must be always wrong and the target must do the right thing to ensure this is true, IMO.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:1205
@@ -1190,1 +1204,3 @@
+              TargetLowering::TypeLegal ||
+            TLI.isTypeLegal(Node->getValueType(i))) &&
            "Unexpected illegal type!");
----------------
How could that be different?!
That looks wrong to have to check for both conditions.


http://reviews.llvm.org/D11438