[llvm-commits] [PATCH] JIT support for ARM

Evan Cheng evan.cheng at apple.com
Mon Aug 27 11:44:35 PDT 2007


On Aug 17, 2007, at 5:28 PM, Raul Fernandes Herbster wrote:

>
>
> 2007/8/17, Evan Cheng <evan.cheng at apple.com>:
> Very good progress. Thanks!
>
> Comments inline.
>
> Evan
>
> ...
>
> Please use abort() instead so it does what's expected in non-debug  
> build.
> OK
>
>
> ...
>
> i s -> is :-)
>
> OK
>
> Also, why the name "ARMCompilationCallbackC"? Is it language specific?
>
> I used the same naming convention of PPCJITInfo.cpp  
> ("PPCCompilationCallbackC"). I can also use naming convention of  
> X86ITInfo.cpp, which uses "X86CompilationCallback2").

No big deal either way. I just want better comments if it's language  
specific in any way.

>
> ...
>
> Does a similar assertion makes sense here?
>
> I dont think so. Such code is only called  for branch and link  
> instruction. In fact, the stub calls a function with MOV and LDR,  
> instead of BL/B (because that problem of 24-bits field).
>
> ...
>
> This is ok.... But I would rather see you refactor  
> getBinaryCodeForInstr() so you can "manufacture" the value by  
> passing it ARM::LDR, ARM::PC, etc.? Do you think that's possible?
>
> I don't think it is possible. If I "manufacture" the value by  
> passing the opcode (ARM::LDR, ARM::PC...), I'll have to implement a  
> big switch table as I did before (you have already comment this  
> solution before). Generating the instructions using its classes it's  
> better.

Ok.

>
>
> Also, in Emitter::getBinaryCodeForInstr():
>
> unsigned Emitter::getBinaryCodeForInstr(const MachineInstr &MI) {
>   const TargetInstrDescriptor *Desc = MI.getInstrDescriptor();
>   const unsigned opcode = MI.getOpcode();
>   unsigned Value = 0xE0000000;
>
> Comments? What is 0xe000000?
>
> Ok. I'll comment. It is an initial instruction mask.

Thx.

>
>
>
>
> Can 12 (and all the magic shift amounts in this function) be defined  
> in ARMII enum? So you add comments there rather than in this code.
>
> You're rigth. I'll define them in ARMII enum.
>
>> +    MCE.emitWordLE(addr);
>>    } else {
>> -    MCE.startFunctionStub(5, 2);
>> -    MCE.emitByte(0xEB);  // branch and link to the corresponding  
>> function addr
>> +    // branch and link to the corresponding function addr
>> +    MCE.startFunctionStub(20, 4);
>> +    MCE.emitWordLE(0xE92D4800); // STMFD SP!, [R11, LR]
>> +    MCE.emitWordLE(0xE28FE004); // ADD LR, PC, #4
>> +    MCE.emitWordLE(0xE51FF004); // LDR PC, [PC,#-4]
>> +    MCE.emitWordLE(addr);
>> +    MCE.emitWordLE(0xE8BD8800); // LDMFD SP!, [R11, PC]
> Ditto.
>
> There are comments for the hexa numbers emitted (MCE.emitWordLE). In  
> such code, I'd better comment instructionsMCE.startFunctionStub().
>>      switch ((ARM::RelocationType)MR->getRelocationType()) {
>>      case ARM::reloc_arm_relative: {
>>        // PC relative relocation
>> -      *((unsigned*)RelocPos) += (unsigned)ResultPtr;
>> +      ResultPtr = ResultPtr-(intptr_t)RelocPos-8;
>> +      if (ResultPtr >= 0)
>> +        *((unsigned*)RelocPos) |= 1 << 23;
>> +      else {
>> +        ResultPtr *= -1;
>> +        *((unsigned*)RelocPos) &= 0xFF7FFFFF;
>
> Please explain what's going on here? :-)
>
> :-). OK. Evan, sorry for not  making comments. In certain functions  
> of files PPCJITInfo.cpp and X86JITInfo.cpp , I did not see any  
> comments so I though that I could ignore them. I'll explain it.

Hehe. I'll go bug folks responsible for those. :-)

>
>
>
>
> Instead of special casing it for LDRD, perhaps add a LB (L bit)  
> class and attach to the other instructions that need it?
> LdFrm/StFrm is used to set bit L. However, the instruction LDRD is  
> an "Enhanced DSP Extension" instruction (page A10-8) and it doesn't  
> have an L bit to be set (neither STRD).
>
>
>  I'd like to see PUWLSH bits modeled more clearly.
>
> In order to model bits PUWLSH, I'll have to create more classes.  
> However, some information about such bits can be retrieved from some  
> other information:
> bit P: there are three classes of addr. I check if it is  
> IndexModePost  to set  it to 1.
> bit U: I have to check immed value (U=1 is possitive, U=0 it is  
> possitive)
> bit L: cI've created classes for it (ARMII::StFrm, ARMII::LdFrm).
> bit S: I've created classes for it  
> (ARMII::DPRImS,ARMII::DPRRegS,ARMII::DPRSoRegS).
> bit W: is set according to addr mode.

Couldn't you just reserve a few bits in TSFlags for these? Then you  
add some classes to set these bits. Take a look at how X86 handles TB,  
XS, etc. I'd rather have them explicitly spelled out for each  
instruction. If this makes sense, please commit your patch first and  
handle PUWLSH as a follow on patch.

Thanks.

Evan

>
>
> -- 
> Raul Fernandes Herbster
> Embedded and Pervasive Computing Laboratory - embedded.dee.ufcg.edu.br
> Electrical Engineering Department - DEE - www.ee.ufcg.edu.br
> Electrical Engineering and Informatics Center - CEEI
> Federal University of Campina Grande - UFCG - www.ufcg.edu.br
> Caixa Postal 10105
> 58109-970 Campina Grande - PB - Brasil  
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

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


More information about the llvm-commits mailing list