[llvm-commits] Trampoline support (pointers nested funtions)

[Duncan Sands]

Hi Evan,

> Some nit picks.
> 
> 1. Please don't use "Chain" for stand for function static chain. It  
> confuses backend guys like me. :-) "Chain" stands for control flow  
> dependency in the backend.

I've replaced Chain with Nest everywhere, eg the attribute is now 'nest'.

> 2. Purely a stylistic thing:
> 
> +SDOperand X86TargetLowering::LowerTRAMPOLINE(SDOperand Op,
> +                                             SelectionDAG &DAG) {
> +  SDOperand Root = Op.getOperand(0);
> +  SDOperand Trmp = Op.getOperand(1); // trampoline
> +  SDOperand FPtr = Op.getOperand(2); // nested function
> +  SDOperand SChn = Op.getOperand(3); // static chain
> +
> +  SrcValueSDNode *TrmpSV = cast<SrcValueSDNode>(Op.getOperand(4));
> +
> +  if (Subtarget->is64Bit()) {
> +    return SDOperand(); // not yet supported
> +  } else {
> 
> If you move the check is64Bit() to the beginning of function, there  
> is no need to nest the part that actually do the work in the "else"  
> clause.

I'd prefer to leave it as it is: this is where the code for 64 bit
support will go, once added.  And since right now codegen will abort
on trampoline lowering on x86-64, I don't think it matters if a few
cycles are wasted before the abort :)  By the way, I think aborting
is the right thing to do: if someone is creating a trampoline, most
likely they are going to use it, i.e. jump to the code it contains.
If we lower trampoline initialization to nothing on architectures that
don't yet support it, then the code will appear to compile fine but will
die very horribly at runtime, by jumping to a bunch of random bytes...

> 3. In X86TargetLowering::LowerTRAMPOLINE():
> +    case CallingConv::X86_StdCall: {
> +      Move = 0xb9; // Pass chain in ECX
> 
> I assume this is the ModR/M byte?

Well, it's MOV32ri.

> Can you refactor ModRMByte() from X86CodeEmitter.cpp (probably also getX86RegNum)
> and use that to calculate this instead?

For the reasons explained in the next paragraph, I've taken a minimal approach.
(1) I've introduced X86CodeEmitter.h, which contains native X86 Register numbers
factored out of X86CodeEmitter.cpp.
(2) In LowerTRAMPOLINE, names like N86::ECX are used to name the register used.
(3) Rather than using 0xb8 and 0xE9, I've introduced symbolic names MOV32ri
and JMP.  I could also get these by doing something like this:
 const X86InstrInfo *TII = ((X86TargetMachine&)getTargetMachine()).getInstrInfo();
 unsigned char MOV32ri = TII->getBaseOpcodeFor(&TII->get(X86::MOV32ri));
But it didn't seem worth it (is there a way to extract the machine opcode
statically?).

> Also, isn't the static chain register described in X86CallingConv.td?

It is, but it's hard to use here.  The problem is that when lowering the
init.trampoline intrinsic you only have a pointer to the target function.
From this pointer you would like to find out which register a certain
parameter will be passed in for that function.  Not so easy!  It's like
having a call instruction without having the arguments.  In order to
exploit X86CallingConv.td, you have to use all the lowering machinery,
which isn't adapted to this case.  For example, you could try to synthesize
a fake call.  Or you could pretend to be lowering the target function.  I
tried it, and it can be done with a lot of horrible hacking.  But it's not
worth it.  It's much simpler to simply grab the calling convention and use
that, which unfortunately means keeping LowerTRAMPOLINE and
X86CallingConv.td in sync.  Personally I can live with that, especially since
I've seen the alternative and it still wakes me up screaming at night :)
But maybe you can see a reasonable way of doing it?

Since I need to map the calling convention to a native X86 register number,
I chose to bypass X86::ECX etc and directly use N86::ECX.  This would be
different if the register number was being extracted from lowering +
CCInfo.

> Magic number is confusing. :-)

Hopefully it's more readable now.  The amount of code factorization is
minimal which is a pity but seems the best choice.

> Looks great otherwise. Thanks!

Thanks for reviewing!

Duncan.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tramp.diff
Type: text/x-diff
Size: 33855 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20070727/fc8ed649/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tramp-gcc.diff
Type: text/x-diff
Size: 4727 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20070727/fc8ed649/attachment-0001.diff>