[PATCH] D11438: Fix x86_64 fp128 calling convention

[Chih-Hung Hsieh via llvm-commits]

chh added a comment.

I mentioned ppcf128 earlier to show how a new floating point type
was added into places that looked like target independent.
f128 for x86_64 is different but also needs to patch into llvm's
type legalization phases. I think llvm's current abstraction of
floating types is mostly good, and I am not ready to make bigger
refactoring yet. Hence, similar to the scattered ppcf128 patches,
we see many f128 type patches.

Some patches looked like optimization, but they are actually
required to compile current libm and match gcc output.
Many ad-hoc optimizations already in LLVM to transform
floating point operations to integer AND/OR/XOR/SHIFT operations,
and they worked up to f80 and expanded f128. Only when I put f128
values into MME registers, some of those ad-hoc optimizations failed.
So I will have to either fix or disable those.
The many strange-looking test cases with i128 types
are derived from actual code in libm.


================
Comment at: include/llvm/Target/TargetLowering.h:1907
@@ -1905,2 +1906,2 @@
 
 private:
----------------
echristo wrote:
> Why do you need to do this?
To keep f128 values in MMX registers, we need FR128RegClass.
But that makes f128 a "legal" type that do not need TypeSoftenFloat.
We still need TypeSoftenFloat actions to convert most f128 operator to
library function calls, so there is explicit call to
 ValueTypeActions.setTypeAction(MVT::f128, TypeSoftenFloat);
in  Target/X86/X86ISelLowering.cpp.
It needs setTypeAction to be accessible to child classes.


================
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 =
----------------
echristo wrote:
> 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.
Added more comments here to make the followed test at line 164 clear.
I am not sure if current semantics of getCommonSubClass should be changed.
So here I handle the requirement at line 164.
Maybe there is a different way to set up new FR128 register classes to make all existing code here work without change. But I have not found that configuration.



================
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);
----------------
echristo wrote:
> SelectionDAG::getTargetConstant? At the least it's not obvious why you need this.
It's similar to ExpandConstantFP and used at line 3535, where the purpose should be easier to understand.
Since new changes avoid breaking f128 and i128 values into two 64-bit registers, we end up with a few cases that were not handled before during ExpandNode.



http://reviews.llvm.org/D11438