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

[Evan Cheng]

On Jul 27, 2007, at 3:38 AM, Duncan Sands wrote:

> 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'.

Thanks.

>
>
>> 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...
>

Ok. It's just a nit pick.

>> 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.

Then it should be 0xb8?

>
>
>> 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.

Please factor out getX86RegNum() as well. Perhaps put them in  
X86RegisterInfo.cpp (since lowering really shouldn't depend on  
codeemitter...) Do getX86RegNum(X86::EAX) rather than make use  
N86::EAX directly.

>
> (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?).

Please go through X86InstrInfo to get the opcode numbers instead of  
hard coding it.

>
>
>> 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?

Seems like a deficiency in CCState class. Chris, your thoughts?

Evan

>
>
> 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.
> <tramp.diff><tramp-gcc.diff>