[PATCH] D11438: Fix x86_64 fp128 calling convention

[Xinliang David Li via llvm-commits]

Your newly proposed change for getCommonSubClass looks cleaner to me -- it
does not change the hasType guarantee of the existing implementation.

David

On Fri, Oct 16, 2015 at 4:54 PM, Chih-hung Hsieh <chh at google.com> wrote:

> Somehow a few previous emails were not seen athttp://reviews.llvm.org/D11438
> Does anyone know the cause of that?
>
>
> As promised in my previous email, I gave more thought about changing
> getCommonSubClass instead of InstrEmitter.cpp. The problem was that
> getCommonSubClass returns FR128 register class as the common sub class
> with VR128. But FR128 does not contain v16i8 and 128-bit vector types.
> I tried to add 128-bit vector types into FR128. That caused many other
> errors that I doubt is the right direction.
>
> Hence, it seems to me that my best solution now is to take care of
> getCommonSubClass inside EmitCopyFromReg of InstrEmitter.cpp,
> but not to change other users of getCommonSubClass.
> The other users do not look like having clear requirement of
> something like ComRC->hasType(VT).
>
> The best alternative I have to my current patch is shown bellow.
> (1) Almost no change to InstrEmitter.cpp, which just passes the VT.SimpleTy
>     to getCommonSubClass, which should then guarantee that returned
>     ComRC will have type VT.
> (2) Change getCommonSubclass to take one more parameter with default
>     value. So other callers do not need any change. When the extra
>     parameter is not MVT::SimpleValueType::Any, returns found
>     common sub class only if it contains the given SimpleValueType.
> (3) getCommonSubclass calls firstCommonClass but it's not the only caller.
>     So change firstCommonClass similarly.
>
> Well, this is actually more change than my current patch.
> So, I am still in favor of my current patch.
>
>
> Index: lib/CodeGen/SelectionDAG/InstrEmitter.cpp
> ===================================================================
> --- lib/CodeGen/SelectionDAG/InstrEmitter.cpp   (revision 247905)
> +++ lib/CodeGen/SelectionDAG/InstrEmitter.cpp   (working copy)
> @@ -139,7 +139,7 @@
>                UseRC = RC;
>              else if (RC) {
>                const TargetRegisterClass *ComRC =
> -                TRI->getCommonSubClass(UseRC, RC);
> +                TRI->getCommonSubClass(UseRC, RC, VT.SimpleTy);
>                // If multiple uses expect disjoint register classes, we emit
>                // copies in AddRegisterOperand.
>                if (ComRC)
>
> Index: include/llvm/Target/TargetRegisterInfo.h
> ===================================================================
> --- include/llvm/Target/TargetRegisterInfo.h    (revision 247905)
> +++ include/llvm/Target/TargetRegisterInfo.h    (working copy)
> @@ -620,10 +620,13 @@
>    }
>
>    /// getCommonSubClass - find the largest common subclass of A and B. Return
> -  /// NULL if there is no common subclass.
> +  /// NULL if there is no common subclass. The common subclass should contain
> +  /// simple value type SVT if it is not the Any type.
>    const TargetRegisterClass *
>    getCommonSubClass(const TargetRegisterClass *A,
> -                    const TargetRegisterClass *B) const;
> +                    const TargetRegisterClass *B,
> +                    const MVT::SimpleValueType SVT =
> +                    MVT::SimpleValueType::Any) const;
>
>    /// getPointerRegClass - Returns a TargetRegisterClass used for pointer
>    /// values.  If a target supports multiple different pointer register classes,
>
> Index: lib/CodeGen/TargetRegisterInfo.cpp
> ===================================================================
> --- lib/CodeGen/TargetRegisterInfo.cpp  (revision 247905)
> +++ lib/CodeGen/TargetRegisterInfo.cpp  (working copy)
> @@ -166,16 +166,24 @@
>  static inline
>  const TargetRegisterClass *firstCommonClass(const uint32_t *A,
>                                              const uint32_t *B,
> -                                            const TargetRegisterInfo *TRI) {
> +                                            const TargetRegisterInfo *TRI,
> +                                            const MVT::SimpleValueType SVT =
> +                                            MVT::SimpleValueType::Any) {
> +  const MVT VT(SVT);
>    for (unsigned I = 0, E = TRI->getNumRegClasses(); I < E; I += 32)
> -    if (unsigned Common = *A++ & *B++)
> -      return TRI->getRegClass(I + countTrailingZeros(Common));
> +    if (unsigned Common = *A++ & *B++) {
> +      const TargetRegisterClass *RC =
> +          TRI->getRegClass(I + countTrailingZeros(Common));
> +      if (SVT == MVT::SimpleValueType::Any || RC->hasType(VT))
> +        return RC;
> +    }
>    return nullptr;
>  }
>
>  const TargetRegisterClass *
>  TargetRegisterInfo::getCommonSubClass(const TargetRegisterClass *A,
> -                                      const TargetRegisterClass *B) const {
> +                                      const TargetRegisterClass *B,
> +                                      const MVT::SimpleValueType SVT) const {
>    // First take care of the trivial cases.
>    if (A == B)
>      return A;
> @@ -184,7 +192,7 @@
>
>    // Register classes are ordered topologically, so the largest common
>    // sub-class it the common sub-class with the smallest ID.
> -  return firstCommonClass(A->getSubClassMask(), B->getSubClassMask(), this);
> +  return firstCommonClass(A->getSubClassMask(), B->getSubClassMask(), this, SVT);
>  }
>
>  const TargetRegisterClass *
>
>
>
> On Tue, Oct 13, 2015 at 4:51 PM, Chih-hung Hsieh <chh at google.com> wrote:
>
>> On Tue, Oct 13, 2015 at 9:21 AM, Quentin Colombet <qcolombet at apple.com>
>> wrote:
>>
>>>
>>> This is interesting observation: fp128 gets split into 2-64-bit
>>> registers.
>>> Looks like we could go incrementally here.
>>> What about:
>>> 1. Changing the android ABI to pass the fp128 via xmm registers, but let
>>> the values live wherever they currently live in the meantime.
>>> 2. Fixing the AsmPrinter.
>>> 3. Fixing the split of the value into 2-64bit registers.
>>>
>>>
>> 2. is probably the simplest and independent part that can go in before
>> the others. The change is only in lib/Target/X86/X86MCInstLower.cpp. If it
>> is left out, llvm can compile libm, but abort on -g in some cases.
>>
>> 1. was what I wanted at the beginning and I don't mind using 2 64-bit
>> registers in other operations if that is possible.
>>
>> However, after I made the following changes for calling convention:
>>    lib/Target/X86/X86RegisterInfo.td
>>    lib/Target/X86/X86CallingConv.td
>>    lib/Target/X86/X86InstrInfo.td
>> I found missing instructions to handle fp128 values load/store in xmm
>> registers, although there were many to process vectors in xmm.
>> So we need some extra load/store instructions in
>>    lib/Target/X86/X86InstrSSE.td
>> Once f128 is 'legal' in register, it seemed easier for llvm to pass it
>> around than converting back-and-forth between 128-bit and 2x64-bit
>> registers.
>> In fact since almost all f128 operations are handled by library
>> functions, the values are mostly passed around and in f128 registers.
>>
>> I didn't try it but I think it would probably require more twist of
>> current llvm code to keep f128 in 2 64-bit registers and pass in xmm.
>> Anyway, if anyone knows how to configure llvm easily, we can try that.
>>
>> Actually most of the changes were required to compile libm, which uses
>> unions to access floating point bits as integers. We need instructions to
>> handle i128 vs f128 issues. Hence, the need of only "partial" logic in
>> LegalizeFloatTypes.cpp.
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151019/129fb179/attachment.html>