<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On May 24, 2013, at 5:47 PM, "Guo, Xiaoyi" <<a href="mailto:Xiaoyi.Guo@amd.com">Xiaoyi.Guo@amd.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; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div class="Section1" style="page: Section1;"><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);">Hi Andy,<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 for doing the memory impact analysis. Attached please find the new set of patches, with all review comments addressed. There’s one exception. The diff can only remove the content of SDNodeOrdering.h, but not the file. You may have to manually remove it using svn command.<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);">[Patch 1/4]: Add new fields and utility classes without changing existing interfaces or functionalities.<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);">[Patch 2/4]: Change SelectionDAG::getXXXNode() interfaces as well as call sites of these functions to pass in SDLoc instead of DebugLoc.<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);">[Patch 3/4]: Remove old IR ordering mechanism and switch to new one. Fix unit test failures.<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);">[Patch 4/4]: Add unit tests.</span></div></div></div></blockquote><div dir="auto"><br></div><div dir="auto">Committed: r182701,r182703,r182704,r182706</div><div dir="auto"><br></div><div dir="auto">Thanks! I just realized I forgot to properly attribute the commit log. I’ll send a message to llvmdev tomorrow to make up for it.</div></div><div dir="auto">-Andy</div><div dir="auto"><br><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; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div class="Section1" style="page: Section1;"><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);"><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,<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);">Xiaoyi<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>Andrew Trick [<a href="mailto:atrick@apple.com" style="color: purple; text-decoration: underline;">mailto:atrick@apple.com</a>]<span class="Apple-converted-space"> </span><br><b>Sent:</b><span class="Apple-converted-space"> </span>Thursday, May 23, 2013 3:41 PM<br><b>To:</b><span class="Apple-converted-space"> </span>Guo, Xiaoyi<br><b>Cc:</b><span class="Apple-converted-space"> </span>Tom Stellard; llvm commits; Stellard, Thomas; Chris Lattner<br><b>Subject:</b><span class="Apple-converted-space"> </span>Re: [patch] DAG nodes IR ordering code re-factoring<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 style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;">On May 21, 2013, at 3:58 PM, Chris Lattner <<a href="mailto:clattner@apple.com" style="color: purple; text-decoration: underline;">clattner@apple.com</a>> wrote:<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;">On May 21, 2013, at 11:43 AM, "Guo, Xiaoyi" <<a href="mailto:Xiaoyi.Guo@amd.com" style="color: purple; text-decoration: underline;">Xiaoyi.Guo@amd.com</a>> wrote:<br><br><br><o:p></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;">Hi Tom,<br><br>Here's the email thread where I explained my motivation and responses from other people.<br><a href="http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-April/061728.html" style="color: purple; text-decoration: underline;">http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-April/061728.html</a><o:p></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><br>I'm not thrilled about making SDNode larger. Have you quantified the memory impact of this change?<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;">The IROrder field fits in padding just before debugLoc. I verified that the bumpPtrAllocator stats don't change when the field is placed optimally.<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;">Incidentally, we already allocate 256 bytes for every SDNode! That's huge compared to IR values and MI instructions. Most of that space is unused for common SDNode types.<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;">If we later want to squeeze more out of the SDNode bits, we can combine IROrder with nodeid which are essentially redundant. Reusing nodeid will take some work though--it's something I'll consider when designing the linearizer to bypass the SD scheduler.<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;">More importantly, moving IROrder to a field actually reduces memory, in addition to being much more efficient. Xaioyi's second patch tracks IR location for most nodes. This tracking will be done all the time (except fast isel)--it's not really optional. We don't want to create hash table entries for most of the SDNodes. Xiaoyi's third patch deletes that giant hash table. On 403.gcc that reduces the memory footprint of SelectBasicBlock by 2MB out of 38MB total. That's pretty good!<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;">Xiaoyi: In your final patch set please do the following<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;">- move the IROrder field to just below debugLoc<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;">- make sure patch #3 deletes SDNodeOrdering.h<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;">- initialize CurInst=0<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;">I don't have any more suggestions. If you've addressed all the review comments, please send a final patch set and I'll check it in. These patches allow pre-RA-sched=source to work as intended. Enabling pre-RA-sched=source is a side effect of enabling the MachineScheduler, so it would be nice to have it working correctly when I do that.<o:p></o:p></div></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;">-Andy<o:p></o:p></div></div></div><span><Mail Attachment></span><span><Mail Attachment></span><span><Mail Attachment></span><span><Mail Attachment></span></div></blockquote></div><br></body></html>