<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">
<br><div><div>On Aug 17, 2007, at 5:28 PM, Raul Fernandes Herbster wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><br><br><div><span class="gmail_quote">2007/8/17, Evan Cheng <<a href="mailto:evan.cheng@apple.com">evan.cheng@apple.com</a>>:</span><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> <div style=""> <div></div><div>Very good progress. Thanks!</div><div><br></div><div>Comments inline.</div><div><br></div><div>Evan</div><div><br>...<br><div><br></div>Please use abort() instead so it does what's expected in non-debug build. </div></div></blockquote><div>OK<br> </div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div style=""><div>...<br><div><br></div>i s -> is :-) </div></div></blockquote><div><br>OK <br></div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div style=""><div>Also, why the name "ARMCompilationCallbackC"? Is it language specific? </div></div></blockquote><div><br>I used the same naming convention of PPCJITInfo.cpp ("PPCCompilationCallbackC"). I can also use naming convention of X86ITInfo.cpp, which uses "X86CompilationCallback2").</div></div></blockquote><div><br class="webkit-block-placeholder"></div>No big deal either way. I just want better comments if it's language specific in any way.</div><div><br><blockquote type="cite"><div><div> </div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div style=""><div>...<br><div><br></div>Does a similar assertion makes sense here? </div></div></blockquote><div><br>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).<br> </div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div style=""><div>...<br><div><br></div>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? </div></div></blockquote><div><br>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. </div></div></blockquote><div><br class="webkit-block-placeholder"></div>Ok.</div><div><br><blockquote type="cite"><div><div><br></div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div style=""><div>Also, in Emitter::getBinaryCodeForInstr():</div><div><br></div> <blockquote style="border: medium none ; margin: 0pt 0pt 0pt 40px; padding: 0px;">unsigned Emitter::getBinaryCodeForInstr(const MachineInstr &MI) {<br> const TargetInstrDescriptor *Desc = MI.getInstrDescriptor();<br> const unsigned opcode = MI.getOpcode();<br> unsigned Value = 0xE0000000;</blockquote><div><br></div><div>Comments? What is 0xe000000?</div></div></blockquote><div><br>Ok. I'll comment. It is an initial instruction mask. </div></div></blockquote><div><br class="webkit-block-placeholder"></div>Thx.</div><div><br><blockquote type="cite"><div><div><br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div style=""><br><div><br></div>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. </div></blockquote><div><br>You're rigth. I'll define them in ARMII enum.<br></div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> <div style=""><div><div><blockquote type="cite"><div>+ MCE.emitWordLE(addr);</div><div> } else {</div><div>- MCE.startFunctionStub(5, 2);</div><div>- MCE.emitByte(0xEB); // branch and link to the corresponding function addr </div><div>+ // branch and link to the corresponding function addr</div><div>+ MCE.startFunctionStub(20, 4);</div><div>+ MCE.emitWordLE(0xE92D4800); // STMFD SP!, [R11, LR]</div><div>+ MCE.emitWordLE(0xE28FE004); // ADD LR, PC, #4 </div><div>+ MCE.emitWordLE(0xE51FF004); // LDR PC, [PC,#-4]</div><div>+ MCE.emitWordLE(addr);</div><div>+ MCE.emitWordLE(0xE8BD8800); // LDMFD SP!, [R11, PC]</div></blockquote>Ditto.</div></div></div></blockquote> <div> <br>There are comments for the hexa numbers emitted (MCE.emitWordLE). In such code, I'd better comment instructionsMCE.startFunctionStub().<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> <div style=""><div><div><blockquote type="cite"><div> switch ((ARM::RelocationType)MR->getRelocationType()) {</div><div> case ARM::reloc_arm_relative: {</div><div> // PC relative relocation</div><div>- *((unsigned*)RelocPos) += (unsigned)ResultPtr; </div><div>+ ResultPtr = ResultPtr-(intptr_t)RelocPos-8;</div><div>+ if (ResultPtr >= 0)</div><div>+ *((unsigned*)RelocPos) |= 1 << 23;</div><div>+ else {</div><div>+ ResultPtr *= -1; </div><div>+ *((unsigned*)RelocPos) &= 0xFF7FFFFF;</div></blockquote><div><br></div>Please explain what's going on here? :-)</div></div></div></blockquote><div><br>:-). 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.</div></div></blockquote><div><br class="webkit-block-placeholder"></div>Hehe. I'll go bug folks responsible for those. :-)</div><div><br><blockquote type="cite"><div><div><br></div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> <div style=""><div><div></div></div></div></blockquote><div> </div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div style=""><div><div> Instead of special casing it for LDRD, perhaps add a LB (L bit) class and attach to the other instructions that need it?</div></div></div></blockquote><div>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). <br> </div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div style=""><div><div> I'd like to see PUWLSH bits modeled more clearly. </div><div><br></div></div></div></blockquote></div>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:<br>bit P: there are three classes of addr. I check if it is IndexModePost to set it to 1. <br>bit U: I have to check immed value (U=1 is possitive, U=0 it is possitive)<br>bit L: cI've created classes for it (ARMII::StFrm, ARMII::LdFrm).<br>bit S: I've created classes for it (ARMII::DPRImS,ARMII::DPRRegS,ARMII::DPRSoRegS). <br>bit W: is set according to addr mode.</blockquote><div><br class="webkit-block-placeholder"></div>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.</div><div><br class="webkit-block-placeholder"></div><div>Thanks.</div><div><br class="webkit-block-placeholder"></div><div>Evan</div><div><br><blockquote type="cite"><br clear="all"><br>-- <br>Raul Fernandes Herbster<br>Embedded and Pervasive Computing Laboratory - <a href="http://embedded.dee.ufcg.edu.br">embedded.dee.ufcg.edu.br</a><br>Electrical Engineering Department - DEE - <a href="http://www.ee.ufcg.edu.br">www.ee.ufcg.edu.br</a><br>Electrical Engineering and Informatics Center - CEEI<br>Federal University of Campina Grande - UFCG - <a href="http://www.ufcg.edu.br">www.ufcg.edu.br</a><br>Caixa Postal 10105 <br>58109-970 Campina Grande - PB - Brasil _______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br></blockquote></div><br></body></html>