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