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

[Raul Fernandes Herbster]

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

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

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.


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



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.

-- 
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20070817/525fa54c/attachment.html>