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
_______________________________________________
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
