On Aug 17, 2007, at 5:28 PM, Raul Fernandes Herbster wrote:
2007/8/17, Evan Cheng <[EMAIL PROTECTED]>:
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@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
_______________________________________________
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
