<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Please don't forget the other comments (e.g. remove LiveVariables because it is never available)<div><br></div><div><br><div><div>On Apr 25, 2013, at 2:07 PM, "Gurd, Preston" <<a href="mailto:preston.gurd@intel.com">preston.gurd@intel.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div lang="EN-US" link="blue" vlink="purple" style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div class="WordSection1" style="page: WordSection1;"><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);">Hello Nadav,<o:p></o:p></span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);"> </span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);">Thanks, once again, for your attentiveness!<o:p></o:p></span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);"> </span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);">I had added the documentation just ahead of each function body, using X86PadShortFunction.cpp and X86VZeroUpper.cpp as models.<o:p></o:p></span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);"> </span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);">I will move the comments for the functions in X86FixupLEAs.cpp to precede the initial declarations.<o:p></o:p></span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);"> </span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);">Preston<o:p></o:p></span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);"> </span></div><div><div style="border-style: solid none none; border-top-color: rgb(181, 196, 223); border-top-width: 1pt; padding: 3pt 0in 0in;"><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><b><span style="font-size: 10pt; font-family: Tahoma, sans-serif;">From:</span></b><span style="font-size: 10pt; font-family: Tahoma, sans-serif;"><span class="Apple-converted-space"> </span>Nadav Rotem [<a href="mailto:nrotem@apple.com">mailto:nrotem@apple.com</a>]<span class="Apple-converted-space"> </span><br><b>Sent:</b><span class="Apple-converted-space"> </span>Thursday, April 25, 2013 4:51 PM<br><b>To:</b><span class="Apple-converted-space"> </span>Gurd, Preston<br><b>Cc:</b><span class="Apple-converted-space"> </span><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br><b>Subject:</b><span class="Apple-converted-space"> </span>Re: [llvm] r180573 - This patch adds the X86FixupLEAs pass, which will reduce instruction<o:p></o:p></span></div></div></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><o:p> </o:p></div><div><blockquote style="margin-top: 5pt; margin-bottom: 5pt;"><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;">+<br>+ virtual const char *getPassName() const { return "X86 Atom LEA Fixup";}<br>+ void seekLEAFixup(MachineOperand& p, MachineBasicBlock::iterator& I,<br>+ MachineFunction::iterator MFI);<br>+ void processInstruction(MachineBasicBlock::iterator& I,<br>+ MachineFunction::iterator MFI);<br>+ RegUsageState usesRegister(MachineOperand& p,<br>+ MachineBasicBlock::iterator I);<br>+ MachineBasicBlock::iterator searchBackwards(MachineOperand& p,<br>+ MachineBasicBlock::iterator& I,<br>+ MachineFunction::iterator MFI);<br>+ MachineInstr* postRAConvertToLEA(MachineFunction::iterator &MFI,<br>+ MachineBasicBlock::iterator &MBBI,<br>+ LiveVariables *LV) const;<br>+<o:p></o:p></div></blockquote><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><o:p> </o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;">Can you please document the declarations of these functions ?<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;">( see<span class="Apple-converted-space"> </span><a href="http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments" style="color: purple; text-decoration: underline;">http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments</a><span class="Apple-converted-space"> </span>)<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><o:p> </o:p></div></div><blockquote style="margin-top: 5pt; margin-bottom: 5pt;"><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><br>+<br>+/// postRAConvertToLEA - if an instruction can be converted to an<span class="apple-converted-space"> </span><br>+/// equivalent LEA, insert the new instruction into the basic block<br>+/// and return a pointer to it. Otherwise, return zero.<br>+MachineInstr *<br>+FixupLEAPass::postRAConvertToLEA(MachineFunction::iterator &MFI,<br>+ MachineBasicBlock::iterator &MBBI,<br>+ LiveVariables *LV) const {<o:p></o:p></div></blockquote><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><o:p> </o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><o:p> </o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;">Preston, LiveVariables are not available in this phase and it is only confusing. Please remove them from the code. <o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><o:p> </o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;">+ LV = getAnalysisIfAvailable<LiveVariables>(); <----- this will always return 0. Did you ever get a LiveVariable instance ?<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><o:p> </o:p></div></div><blockquote style="margin-top: 5pt; margin-bottom: 5pt;"><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;">+ // Process all basic blocks.<br>+ for (MachineFunction::iterator I = Func.begin(), E = Func.end(); I != E; ++I)<br>+ processBasicBlock(Func, I);<br>+ DEBUG(dbgs() << "End X86FixupLEAs\n";);<br>+<br>+ return true;<br>+}<o:p></o:p></div></blockquote><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><o:p> </o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><o:p> </o:p></div></div><div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;">This function always returns true. You probably only want to return true of the code changed. <o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><o:p> </o:p></div></div></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;">Can you reduce the size of the test cases ? Ideally I would like to see a single-basic block function. You only need 5-instructions or so. Why did you include such long tests ?</div></div></div></div></blockquote></div><br></div></body></html>