[PATCH] D11438: Fix x86_64 fp128 calling convention

[Eric Christopher via llvm-commits]

echristo added a comment.

Hi Chh,

I've left a few inline comments on this, but I've got a couple of larger meta comments first:

I see you modeled this after the ppcf128 support. ppcf128 is a special float format and not just f128 on a particular architecture, so why is this correct here? I'm not sure it is at the moment.

This patch can also be broken up into separate patches, at the least things like the instruction printing, and there may be more splits you can make (like optimization versus correctness).

These are some amazing testcases btw. Thanks for making sure it's so well tested!

Thanks!

-eric


================
Comment at: include/llvm/Target/TargetLowering.h:1907
@@ -1905,2 +1906,2 @@
 
 private:
----------------
Why do you need to do this?

================
Comment at: lib/CodeGen/SelectionDAG/InstrEmitter.cpp:141-142
@@ -140,2 +140,4 @@
             else if (RC) {
+              // TODO: Make sure that
+              // ComRC->hasType(Node->getSimpleValueType(ResNo));
               const TargetRegisterClass *ComRC =
----------------
These need to be real comments, not TODOs. I.e. What's the todo mean? why is it important? Why isn't it done now?

Ditto for the rest of them that look like this.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:291
@@ +290,3 @@
+/// Expands the Constant node to a load from the constant pool.
+SDValue SelectionDAGLegalize::ExpandConstant(ConstantSDNode *CP) {
+  SDLoc dl(CP);
----------------
SelectionDAG::getTargetConstant? At the least it's not obvious why you need this.


http://reviews.llvm.org/D11438