[PATCH] D11438: Fix x86_64 fp128 calling convention

[Xinliang David Li via llvm-commits]

Quentin, can you help do a quick review of the patch and give
suggestions on high level direction of the patch?

thanks,

David

On Wed, Oct 7, 2015 at 3:03 PM, Chih-Hung Hsieh <chh at google.com> wrote:
> chh added inline comments.
>
> ================
> Comment at: lib/CodeGen/SelectionDAG/InstrEmitter.cpp:169
> @@ -164,1 +168,3 @@
> +    // If TRI->getCommonSubClass(UseRC, RC) returns some register class,
> +    // e.g. FR128, that does not contain VT, it should not be used for DstRC.
>      DstRC = UseRC;
> ----------------
> davidxl wrote:
>> What is VT in this case? What reg class is ComRC here? How about the DstRC below TLI->getRegClassFor(VT)?
> One of the test cases showing this problem is
> test/CodeGen/X86/avx512-calling-conv.ll,
> compiled with -mtriple=x86_64-apple-darwin -mcpu=knl
>
> VT is v16i8 (23)
> ComRC is the new FR128RegClassID (74)
> DstRC is not set yet, but the correct
> TLI->getRegClassFor(VT) is VR128RegClassID (75)
>
> It means that my current definition of FR128 has registers in VR128,
> and ComRC could be FR128 where VR128 was the only candidate before.
> But of course FR128 with my current instruction selection patterns won't
> work for v16i8, and it should not.
> * I tried in my early development of this patch to store f128 value in existing register classes, without a new FR128. But that approach has even more troubles.
> * I also tried to copy some VR128 instruction patterns to FR128 to make this work without this patch in EmitCopyFromReg. But that requires a lot of duplication in instruction selection pattern, and I had not even resolved all the problems.
> * One thing I think might be possible, as my original TODO item, is to change the semantic of getCommonSubClass to guarantee ComRC->hasType(VT), or an extra flag for getCommonSubclass to guarantee ComRC->hasType(VT). I was hoping to break this patch into smaller pieces, and those refactoring tasks seem to be good candidates for follow up patches.
>
>
>
> ================
> 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);
> ----------------
> davidxl wrote:
>> Change the name to ExpandConstantFP128?
> Although it is triggered now only for f128 type, but it should work for other types say f80, f96, etc. Actually, the constant value is not floating type, but an integer (binary bits) of the floating point value from CP->getConstantIntValue.
>
>
> ================
> Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:3535
> @@ +3534,3 @@
> +    ConstantSDNode *CP = cast<ConstantSDNode>(Node);
> +    Results.push_back(ExpandConstant(CP));
> +    break;
> ----------------
> davidxl wrote:
>> I don't see what expected EVTs are checked in this method?
> I think there is no target-independent type restriction here.
> I see TLI.isFPImmLegal check in ISD::ConstantFP, but don't see similar limitation for non-FP types.
> ExpandConstant could have more value range limit if necessary.
>
>
> ================
> Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:64
> @@ +63,3 @@
> +  // return false, otherwise, mark this node as done and return true.
> +  if (KeepFloat)
> +    switch (N->getOpcode()) {
> ----------------
> davidxl wrote:
>> Does the following change affects other types other than f128?
> Not now. Only x86_64 f128 is configured this way. But in the future new configurations can use this logic to keep value legal (in register) but with soften operators.
>
>
>
> http://reviews.llvm.org/D11438
>
>
>