RegCall - Handling long double arguments

[oren_ben_simhon]

Summary

The change is part of RegCall calling convention support for LLVM.
Long double (f80) requires special treatment as the first f80 parameter is saved in FP0 (floating point stack).
This review present the change and the corresponding tests.

[oren_ben_simhon]

A kindly reminder. I will appreciate your inputs on the review.

[ahatanak]

lib/Target/X86/X86FloatingPoint.cpp

485 ↗

(On Diff #76408)

I guess this is needed only for the entry basic block? If so, perhaps you can move this to runOnMachineFunction so that we don't have to check whether the calling convention is regcall every time a basic block is visited.

956 ↗

(On Diff #76408)

You can move this outside the loop since the calling convention and StackTop don't change inside the loop.

973 ↗

(On Diff #76408)

I'm not sure what this comment means.

It seems like you are clearing StackTop because the first FP register can be used to pass an argument when the calling convention is regcall. Is that correct?

[oren_ben_simhon]

Implementing comments posted until 11/07

[oren_ben_simhon]

lib/Target/X86/X86FloatingPoint.cpp

973 ↗

(On Diff #76408)

The problem occurs when FP0 was passed as argument and we try to return a parameter in FP0 as well.
In that case the StackTop points to 1 and we get to the following loop and we call pushreg(0) when FP0 is already assigned and StackTop is 1.

So what actually happened before pushreg:

Stack[] = { 0, ...}
RegMap[] = { 0, ...}

While after pushreg[0] (No stacktop reset):

Stack[] = { 0, 1..}
RegMap[] = { 1...}

The issue causes debug assertion in line 169 to fail because then regmap[stack[0]] != 0.

I changed the way I handle the issue.
Instead of reseting the StackTop to zero, I do not push registers if they were already pushed into the stack.

[ahatanak]

lib/Target/X86/X86FloatingPoint.cpp

348 ↗

(On Diff #77181)

If this is just assigning 0 (FP0), maybe you can just explicitly do so rather than assigning Stack[0] (which is OK because it's initialized to 0 using memset) to make your intent clear?

973 ↗

(On Diff #76408)

Wouldn't this push an extra register when a function taking a long double argument and returning complex long double were called (in which case N==2 because it's returned in st0 and st1)?

[oren_ben_simhon]

lib/Target/X86/X86FloatingPoint.cpp

973 ↗

(On Diff #76408)

AFAIK, Long doubles are saved inside a single ST register (size of 80 bit).
So in the case of one FP argument and one FP retruned value, I expect N to be 1.
Meaning, The value will be returned in ST0.

[oren_ben_simhon]

Implementing comments posted until 11/08

[ahatanak]

lib/Target/X86/X86FloatingPoint.cpp

973 ↗

(On Diff #76408)

When the following code is compiled with clang, the complex long double return value is returned in two registers. N is 2 and StackTop is 1 when the call to foo3 is visited in FPS::handleCall:

typedef long double _Complex CLD;
long double d0;

CLD __regcall foo3(long double a);

CLD __regcall  foo2(void) {
  return foo3(d0);
}

$ clang -cc1 -ffreestanding -triple=x86_64-pc-linux-gnu -o - -S test1.c

Do you know why this is happening?

[oren_ben_simhon]

lib/Target/X86/X86FloatingPoint.cpp

973 ↗

(On Diff #76408)

You are correct. It acts according the default x64 calling convention, which returns COMPLEX_X87 in two registers.
The updated condition below will not break this behaviour and still two registers will be assigned.

[ahatanak]

lib/Target/X86/X86CallingConv.td

160 ↗

(On Diff #77320)

Since RC.FP includes only FP0, this indicates that only one register is used to return a value even though two registers are used when returning complex long double. I think this currently works only because it falls backs on RetCC_X86Common to find the return register for the latter half of the complex long double return value.

Wouldn't it be better to include both FP0 and FP1 in RC.FP?

lib/Target/X86/X86FloatingPoint.cpp

973 ↗

(On Diff #76408)

If StackTop == 1 and N ==2, this pushes FP0 and FP1 to the stack in the reverse order (FP0 is on the stack already and then pushReg(1) is called). I think you have to reset StackTop and clear Stack and RegMap before pushing all return registers to the stack.

[ahatanak]

lib/Target/X86/X86CallingConv.td

160 ↗

(On Diff #77320)

I mean you can use separate register lists for the argument and return registers.

[oren_ben_simhon]

Implemented comments posted until 11/17

[ahatanak]

LGTM

llvm/trunk/lib/Target/X86/X86CallingConv.td

: CCIf<!strconcat("static_cast<const X86Subtarget&>"
            A>;
 
 // Register classes for RegCall
 class RC_X86_RegCall {
   list<Register> GPR_8 = [];
   list<Register> GPR_16 = [];
   list<Register> GPR_32 = [];
   list<Register> GPR_64 = [];
+  list<Register> FP_CALL = [FP0];
+  list<Register> FP_RET = [FP0, FP1];
   list<Register> XMM = [];
   list<Register> YMM = [];
   list<Register> ZMM = [];
 }
 
 // RegCall register classes for 32 bits
 def RC_X86_32_RegCall : RC_X86_RegCall {
   let GPR_8 = [AL, CL, DL, DIL, SIL];
// Handles byval parameters.
     CCIfSubtarget<"is64Bit()", CCIfType<[i64], 
       CCAssignToReg<RC.GPR_64>>>,
     CCIfSubtarget<"is32Bit()", CCIfType<[i64], 
       CCCustom<"CC_X86_32_RegCall_Assign2Regs">>>,
 
     // TODO: Handle the case of mask types (v*i1)
     CCIfType<[v8i1, v16i1, v32i1], CCCustom<"CC_X86_RegCall_Error">>,
 
-    // TODO: Handle the case of long double (f80)
-    CCIfType<[f80], CCCustom<"CC_X86_RegCall_Error">>,
-
     // float, double, float128 --> XMM
     // In the case of SSE disabled --> save to stack
     CCIfType<[f32, f64, f128], 
       CCIfSubtarget<"hasSSE1()", CCAssignToReg<RC.XMM>>>,
 
+    // long double --> FP
+    CCIfType<[f80], CCAssignToReg<RC.FP_CALL>>,
+
     // __m128, __m128i, __m128d --> XMM
     // In the case of SSE disabled --> save to stack
     CCIfType<[v16i8, v8i16, v4i32, v2i64, v4f32, v2f64], 
       CCIfSubtarget<"hasSSE1()", CCAssignToReg<RC.XMM>>>,
 
     // __m256, __m256i, __m256d --> YMM
     // In the case of SSE disabled --> save to stack
     CCIfType<[v32i8, v16i16, v8i32, v4i64, v8f32, v4f64], 
// Handles byval parameters.
     CCIfType<[i64, f64], CCAssignToStack<8, 4>>,
 
     // MMX type gets 8 byte slot in stack , while alignment depends on target
     CCIfSubtarget<"is64Bit()", CCIfType<[x86mmx], CCAssignToStack<8, 8>>>,
     CCIfType<[x86mmx], CCAssignToStack<8, 4>>,
 
     // float 128 get stack slots whose size and alignment depends 
     // on the subtarget.
-    CCIfType<[f128], CCAssignToStack<0, 0>>,
+    CCIfType<[f80, f128], CCAssignToStack<0, 0>>,
 
     // Vectors get 16-byte stack slots that are 16-byte aligned.
     CCIfType<[v16i8, v8i16, v4i32, v2i64, v4f32, v2f64], 
       CCAssignToStack<16, 16>>,
 
     // 256-bit vectors get 32-byte stack slots that are 32-byte aligned.
     CCIfType<[v32i8, v16i16, v8i32, v4i64, v8f32, v4f64], 
       CCAssignToStack<32, 32>>,
CCIfSubtarget<"is64Bit()", CCIfType<[i64],
       CCAssignToReg<RC.GPR_64>>>,
     CCIfSubtarget<"is32Bit()", CCIfType<[i64], 
       CCCustom<"CC_X86_32_RegCall_Assign2Regs">>>,
 
     // TODO: Handle the case of mask types (v*i1)
     CCIfType<[v8i1, v16i1, v32i1], CCCustom<"CC_X86_RegCall_Error">>,
 
     // long double --> FP
-    CCIfType<[f80], CCAssignToReg<[FP0]>>,
+    CCIfType<[f80], CCAssignToReg<RC.FP_RET>>,
 
     // float, double, float128 --> XMM
     CCIfType<[f32, f64, f128], 
       CCIfSubtarget<"hasSSE1()", CCAssignToReg<RC.XMM>>>,
 
     // __m128, __m128i, __m128d --> XMM
     CCIfType<[v16i8, v8i16, v4i32, v2i64, v4f32, v2f64], 
       CCIfSubtarget<"hasSSE1()", CCAssignToReg<RC.XMM>>>,

llvm/trunk/lib/Target/X86/X86FloatingPoint.cpp

#endif
     void pushReg(unsigned Reg) {
       assert(Reg < NumFPRegs && "Register number out of range!");
       if (StackTop >= 8)
         report_fatal_error("Stack overflow!");
       Stack[StackTop] = Reg;
       RegMap[Reg] = StackTop++;
     }
 
+    // popReg - Pop a register from the stack.
+    void popReg() {
+      if (StackTop == 0)
+        report_fatal_error("Cannot pop empty stack!");
+      RegMap[Stack[--StackTop]] = ~0;     // Update state
+    }
+
     bool isAtTop(unsigned RegNo) const { return getSlot(RegNo) == StackTop-1; }
     void moveToTop(unsigned RegNo, MachineBasicBlock::iterator I) {
       DebugLoc dl = I == MBB->end() ? DebugLoc() : I->getDebugLoc();
       if (isAtTop(RegNo)) return;
 
       unsigned STReg = getSTReg(RegNo);
       unsigned RegOnTop = getStackEntry(0);
 
bool FPS::runOnMachineFunction(MachineFunction &MF) {
 
   StackTop = 0;
 
   // Process the function in depth first order so that we process at least one
   // of the predecessors for every reachable block in the function.
   df_iterator_default_set<MachineBasicBlock*> Processed;
   MachineBasicBlock *Entry = &MF.front();
 
+  LiveBundle &Bundle =
+    LiveBundles[Bundles->getBundle(Entry->getNumber(), false)];
+  
+  // In regcall convention, some FP registers may not be passed through
+  // the stack, so they will need to be assigned to the stack first
+  if ((Entry->getParent()->getFunction()->getCallingConv() ==
+    CallingConv::X86_RegCall) && (Bundle.Mask && !Bundle.FixCount)) {
+    // In the register calling convention, up to one FP argument could be 
+    // saved in the first FP register.
+    // If bundle.mask is non-zero and Bundle.FixCount is zero, it means
+    // that the FP registers contain arguments.
+    // The actual value is passed in FP0.
+    // Here we fix the stack and mark FP0 as pre-assigned register.
+    assert((Bundle.Mask & 0xFE) == 0 &&
+      "Only FP0 could be passed as an argument");
+    Bundle.FixCount = 1;
+    Bundle.FixStack[0] = 0;
+  }
+
   bool Changed = false;
   for (MachineBasicBlock *BB : depth_first_ext(Entry, Processed))
     Changed |= processBasicBlock(MF, *BB);
 
   // Process any unreachable blocks in arbitrary order now.
   if (MF.size() != Processed.size())
     for (MachineBasicBlock &BB : MF)
       if (Processed.insert(&BB).second)

 /// into the instruction itself if possible.  The iterator is left pointing to
 /// the last instruction, be it a new pop instruction inserted, or the old
 /// instruction if it was modified in place.
 ///
 void FPS::popStackAfter(MachineBasicBlock::iterator &I) {
   MachineInstr &MI = *I;
   const DebugLoc &dl = MI.getDebugLoc();
   ASSERT_SORTED(PopTable);
-  if (StackTop == 0)
-    report_fatal_error("Cannot pop empty stack!");
-  RegMap[Stack[--StackTop]] = ~0;     // Update state
+
+  popReg();
 
   // Check to see if there is a popping version of this instruction...
   int Opcode = Lookup(PopTable, I->getOpcode());
   if (Opcode != -1) {
     I->setDesc(TII->get(Opcode));
     if (Opcode == X86::UCOM_FPPr)
       I->RemoveOperand(0);
   } else {    // Insert an explicit pop

 
 
 //===----------------------------------------------------------------------===//
 // Instruction transformation implementation
 //===----------------------------------------------------------------------===//
 
 void FPS::handleCall(MachineBasicBlock::iterator &I) {
   unsigned STReturns = 0;
+  const MachineFunction* MF = I->getParent()->getParent();
 
   for (const auto &MO : I->operands()) {
     if (!MO.isReg())
       continue;
 
     unsigned R = MO.getReg() - X86::FP0;
 
     if (R < 8) {
+      if (MF->getFunction()->getCallingConv() != CallingConv::X86_RegCall) {
         assert(MO.isDef() && MO.isImplicit());
+      }
+
       STReturns |= 1 << R;
     }
   }
 
   unsigned N = countTrailingOnes(STReturns);
 
   // FP registers used for function return must be consecutive starting at
-  // FP0.
+  // FP0
   assert(STReturns == 0 || (isMask_32(STReturns) && N <= 2));
 
+  // Reset the FP Stack - It is required because of possible leftovers from
+  // passed arguments. The caller should assume that the FP stack is 
+  // returned empty (unless the callee returns values on FP stack).
+  while (StackTop > 0)
+    popReg();
+
   for (unsigned I = 0; I < N; ++I)
     pushReg(N - I - 1);
 }
 
 /// If RET has an FP register use operand, pass the first one in ST(0) and
 /// the second one in ST(1).
 void FPS::handleReturn(MachineBasicBlock::iterator &I) {
   MachineInstr &MI = *I;

llvm/trunk/lib/Target/X86/X86ISelLowering.cpp

if (VA.isRegLoc()) {
         if (RegVT == MVT::i32)
           RC = &X86::GR32RegClass;
         else if (Is64Bit && RegVT == MVT::i64)
           RC = &X86::GR64RegClass;
         else if (RegVT == MVT::f32)
           RC = &X86::FR32RegClass;
         else if (RegVT == MVT::f64)
           RC = &X86::FR64RegClass;
+        else if (RegVT == MVT::f80)
+          RC = &X86::RFP80RegClass;
         else if (RegVT == MVT::f128)
           RC = &X86::FR128RegClass;
         else if (RegVT.is512BitVector())
           RC = &X86::VR512RegClass;
         else if (RegVT.is256BitVector())
           RC = &X86::VR256RegClass;
         else if (RegVT.is128BitVector())
           RC = &X86::VR128RegClass;

llvm/trunk/test/CodeGen/X86/avx512-regcall-NoMask.ll

 ; Test regcall when passing/retrieving double
 define x86_regcallcc double @test_CallargRetDouble(double %a)  {
   %b = fadd double 1.0, %a
   %c = call x86_regcallcc double @test_argRetDouble(double %b)
   %d = fadd double 1.0, %c
   ret double %d
 }
 
+; X32: test_argRetf80
+; X32-NOT: fldt
+; X32: fadd %st(0), %st(0)
+; X32: retl
+
+; WIN64: test_argRetf80
+; WIN64-NOT: fldt
+; WIN64: fadd %st(0), %st(0)
+; WIN64: retq
+
+; Test regcall when receiving/returning long double
+define x86_regcallcc x86_fp80 @test_argRetf80(x86_fp80 %a0) nounwind {
+  %r0 = fadd x86_fp80 %a0, %a0
+  ret x86_fp80 %r0
+}
+
+; X32: test_CallargRetf80
+; X32-NOT: fldt
+; X32: fadd %st({{[0-7]}}), %st({{[0-7]}})
+; X32: call{{.*}}   {{.*}}test_argRetf80
+; X32: fadd{{.*}} %st({{[0-7]}})
+; X32: retl
+
+; WIN64: test_CallargRetf80
+; WIN64-NOT: fldt
+; WIN64: fadd %st({{[0-7]}}), %st({{[0-7]}})
+; WIN64: call{{.*}}   {{.*}}test_argRetf80
+; WIN64: fadd{{.*}} %st({{[0-7]}})
+; WIN64: retq
+
+; Test regcall when passing/retrieving long double
+define x86_regcallcc x86_fp80 @test_CallargRetf80(x86_fp80 %a)  {
+  %b = fadd x86_fp80 %a, %a
+  %c = call x86_regcallcc x86_fp80 @test_argRetf80(x86_fp80 %b)
+  %d = fadd x86_fp80 %c, %c
+  ret x86_fp80 %d
+}
+
 ; X32-LABEL:  test_argRetPointer:
 ; X32:        incl %eax
 ; X32:        ret{{.*}}
 
 ; WIN64-LABEL:  test_argRetPointer:
 ; WIN64:        incl %eax
 ; WIN64:        ret{{.*}}