[llvm-commits] [LLVMdev] x86 calling conventions refactoring

[Gordon Henriksen]

Anton,

Going back to the list now that it's working. :)

On 2008-01-04, at 08:22, Anton Korobeynikov wrote:

> Hello, Gordon.
>
> Here goes quick review.
>
>
>> +// Determines whether a CALL node uses struct return semantics.
>> +static bool CallIsStructReturn(SDOperand Op)
> I like these predicates, because later we can move them to the
> autogenerated code.

Yes!

>> +  // Decoarate the function name.
> Typo

Okay.

>> +  if (Is64Bit && CC == CallingConv::X86_FastCall &&
>> +      !Subtarget->isTargetCygMing() && !Subtarget- 
>> >isTargetWindows() &&
>> +      (StackSize & 7) == 0)
and
>> +  if (!Is64Bit && CC == CallingConv::X86_FastCall &&
>> +      !Subtarget->isTargetCygMing() && !Subtarget- 
>> >isTargetWindows() &&
>> +      (NumBytes & 7) == 0)
>> +    NumBytes += 4;

The former is also a typo. It should definitely be !Is64Bit.

> That's pretty interesting.

Indeed. I made some mechanical transformations where the rationale or  
invariants were unclear.

> The source code was:
> -  if (!Subtarget->isTargetCygMing() && !Subtarget- 
> >isTargetWindows()) {
> -    // Make sure the instruction takes 8n+4 bytes to make sure the
> start of the
> -    // arguments and the arguments after the retaddr has been  
> pushed are
> -    // aligned.
> -    if ((NumBytes & 7) == 0)
> -      NumBytes += 4;
> -  }
> -
> Actually this is a hunk from the times, when fastcall and fastcc  
> shared the same LowerFORMAL_ARGUMENTS call.
History tends to repeat itself. :)
> The "funny" thing is that fastcall can be seen only on windows (32  
> bit!) targets, so the condition is twice bogus. Most probably the  
> whole hunk should be just eliminated.

If you're sure, I'll happily delete the lot. But it doesn't look  
necessarily inert to me once the typo is fixed.

I think this should be sunken into GetAlignedArgumentStackSize if it  
remains, since it's always called something like this:

    unsigned NumBytes = CCInfo.getNextStackOffset();
    if (CC == CallingConv::Fast)
      NumBytes = GetAlignedArgumentStackSize(NumBytes, DAG);

+  // Make sure the instruction takes 8n+4 bytes to make sure the  
start of the
+  // arguments and the arguments after the retaddr has been pushed  
are aligned.
+  if (!Is64Bit && CC == CallingConv::X86_FastCall &&
+      !Subtarget->isTargetCygMing() && !Subtarget->isTargetWindows() &&
+      (NumBytes & 7) == 0)
+    NumBytes += 4;

Where GetAlignedArgumentStackSize is documented to…

/// GetAlignedArgumentStackSize - Make the stack size align e.g 16n +  
12 aligned
/// for a 16 byte align requirement.

And is coded fairly generically, although I didn't dig deep enough to  
figure out whether GetAlignedArgumentStackSize would DTRT in this  
particular case. (getNextStackOffset won't; it's just an accumulator.)  
Ultimately, I think GetAlignedArgumentStackSize should do this bit's  
job, and the lot should be sunk into getNextStackOffset—but I was  
trying to be safe and incremental.


>> +  if (IsCalleePop(Op)) {
>> +    BytesToPopOnReturn  = StackSize; // Callee pops everything.
> Maybe it will be better to make function return amount of bytes to  
> pop and fold ArgsAreStructReturn() call there?

Maybe something like that. This logic is essentially duplicated in  
LowerCALL and LowerFORMAL_ARGUMENTS, except for the SDNode passed to  
IsStructReturn is of differing type. (IsCalleePop just happens to be  
looking at bits of FORMAL_ARGUMENTS or CALL nodes which are  
structurally identical; CallIsStructReturn and ArgsAreStructReturn  
don't have that luxury, though they could consult the SDNode's type.)  
The relevant passages:

   // Some CCs need callee pop.
   if (IsCalleePop(Op)) {
     BytesToPopOnReturn  = StackSize; // Callee pops everything.
     BytesCallerReserves = 0;
   } else {
     BytesToPopOnReturn  = 0; // Callee pops nothing.
     // If this is an sret function, the return should pop the hidden  
pointer.
     if (!Is64Bit && ArgsAreStructReturn(Op))
       BytesToPopOnReturn = 4;
     BytesCallerReserves = StackSize;
   }
-----------------------------------------------------------------------------
   // Create the CALLSEQ_END node.
   unsigned NumBytesForCalleeToPush;
   if (IsCalleePop(Op))
     NumBytesForCalleeToPush = NumBytes;    // Callee pops everything
   else if (!Is64Bit && CallIsStructReturn(Op))
     // If this is is a call to a struct-return function, the callee
     // pops the hidden struct pointer, so we have to push it back.
     // This is common for Darwin/X86, Linux & Mingw32 targets.
     NumBytesForCalleeToPush = 4;
   else
     NumBytesForCalleeToPush = 0;  // Callee pops nothing.

>> +    if (IsTailCall || !Is64Bit ||
>> +        getTargetMachine().getCodeModel() != CodeModel::Large)
>> +      Callee = DAG.getTargetExternalSymbol(S->getSymbol(),
>> getPointerTy());
> I myself feel somehow inconvenient to have such condition here  
> (mixing predicates of different sorts).

I don't like repeatedly consulting either PerformTailCallOpt/ 
IsTailCall or !Subtarget->is64Bit(). Rather, I'd prefer to have a  
table-driven system. I think that's a followup patch, though; this one  
is already too large.

> Maybe it will be worth to keep 64 bit variant separate. I don't  
> know. Evan?


The 64-bit code diverged only in a very few details.

— Gordon

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20080104/88990417/attachment.html>