<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Hi John,<div><br></div><div>It looks pretty good. Thanks for working on this. Some comments:</div><div><br></div><div>1. Some of the functions that you introduced, e.g. stringifyCSRegSet probably ought to be "static" and ifdef'ed out when NDEBUG is defined.</div><div><br></div><div>2. + // DEBUG </div><div>+ if (! MBB->empty() && ! CSRUsed[MBB].intersects(restore)) { </div><div>+ MachineInstr* MI = BeforeI; </div><div>+ DOUT << "adding restore after "; </div><div>+ DEBUG(MI->dump()); </div><div>+ } else { </div><div>+ DOUT << "adding restore to beginning of " </div><div>+ << getBasicBlockName(MBB) << "\n"; </div><div>+ } </div><div>+ // DEBUG</div><div><br></div><div>Code like this should also be inside ifndef NDEBUG.</div><div><br></div><div>3. It can still use more refactoring. :-)</div><div><br></div><div>4. clearSets().</div><div>It's not clear what sets it's clearing. Perhaps name it something like clearShrinkWrapData()?</div><div><br></div><div>5</div><div>+void PEI::placeSpillsAndRestores(MachineFunction &Fn) { </div><div>+ </div><div>+ DOUT << "Computing SAVE, RESTORE sets\n"; </div><div>+ </div><div>+ // If not shrink wrapping, all spills go into entry block, </div><div>+ // all restores in return blocks.</div><div>+ if (! ShrinkWrapping) { </div><div>+ // Do spills in the entry block.</div><div><br></div><div>The shrink wrap version probably should go to its own function. Otherwise, it should exit early when the non-shrink wrapping version is done. That reduces nesting and please those of us who are a bit picky.</div><div><br></div><div>6.</div><div><div>+ // Save entry block, return blocks. </div><div>+ if (MBB->pred_size() == 0) </div><div>+ entryBlock = MBB;</div><div><br></div><div>Entry block is the Fn.front().</div><div><br></div><div>7.</div><div><div>+ if (!MBB->empty() && MBB->back().getDesc().isReturn()) </div><div>+ returnBlocks.push_back(MBB);</div><div><br></div><div>PEI::insertPrologEpilogCode also traverse MBBs and get at return blocks. So these probably ought to be shared, i.e. returnBlocks should be an ivar and determined early in PEI.</div><div><br></div><div>8.</div><div><div>+ for (MachineBasicBlock::iterator I = MBB->begin(); I != MBB->end(); ++I) { </div><div>+ for (unsigned i = 0, e = CSI.size(); i != e; ++i) { </div><div>+ unsigned Reg = CSI[i].getReg(); </div><div>+ // If instruction I reads or modifies Reg, add it to UsedCSRegs, </div><div>+ // CSRUsed map for the current block. </div><div>+ if (I->readsRegister(Reg, TRI) || I->modifiesRegister(Reg, TRI)) {</div><div><br></div><div>readsRegister and modifiesRegister both scan the operands. It's probably better to manually walk through the operands.</div><div><br></div><div>9.</div><div><div>+ // If all uses of CSRegs are in the entry block, there is nothing </div><div>+ // to shrink wrap: spills go in entry block, restores go in exiting </div><div>+ // blocks, turn off shrink wrapping. </div><div>+ if (allCSRUsesInEntryBlock) { </div><div>+ ShrinkWrapping = false; </div><div>+ DOUT << "All uses of CSRegs are in entry block, nothing to do.\n"; </div><div>+ } </div><div>+ // If we have decided not to shrink wrap, just return now. </div><div>+ if (! ShrinkWrapping) </div><div>+ return true;</div><div><br></div><div>Why not just return inside if (allCSRUsesInEntryBlock)?</div><div><br></div><div>10.</div><div>+bool PEI::calculateUsedAnticAvail(MachineFunction &Fn) {</div><div>...</div><div>+ // Calculate AnticIn, AnticOut using post-order traversal of MCFG.</div><div><div>+ for (po_iterator<MachineBasicBlock*> </div><div>+ MBBI = po_begin(Fn.getBlockNumbered(0)), </div><div>+ MBBE = po_end(Fn.getBlockNumbered(0)); MBBI != MBBE; ++MBBI) { </div><div>+ MachineBasicBlock* MBB = *MBBI;</div><div>...</div><div><div>+ // Calculate Avail{In,Out} via top-down walk of Machine dominator tree. </div><div>+ for (df_iterator<MachineDomTreeNode*> DI = df_begin(DT.getRootNode()), </div><div>+ E = df_end(DT.getRootNode()); DI != E; ++DI) { </div><div><br></div><div><br></div><div>Later in </div><div><div>+/// placeSpillsAndRestores - decide which MBBs need spills, restores </div><div>+/// of CSRs. </div><div>+/// </div><div>+void PEI::placeSpillsAndRestores(MachineFunction &Fn) {</div><div>...</div><div><div>+ // Calculate CSRRestore using post-order traversal of Machine-CFG. </div><div>+ for (po_iterator<MachineBasicBlock*> </div><div>+ MBBI = po_begin(Fn.getBlockNumbered(0)), </div><div>+ MBBE = po_end(Fn.getBlockNumbered(0)); MBBI != MBBE; ++MBBI) { </div><div><br></div><div>This seem to be doing traversal at least one too many times? Can this be improved?</div></div></div></div></div><div><br></div><div>11. Can you explain a bit more about AnticIn, AvailIn, etc.?</div><div><br></div><div>12.</div><div>Let's worry about edge splitting for a later time. :-)</div></div></div></div></div><div><br></div><div>13. After the code is cleaned up, we should consider checking it in and try it out as llcbeta. Do you have any idea of its compile time impact?</div><div><br></div><div>Thanks,</div><div><br></div><div>Evan</div><div><br><div><div>On Mar 4, 2009, at 7:57 PM, John Mosby wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">Here is an updated patch for shrink wrapping with:<div><br></div><div>- spills/restores done with stack slot stores/loads</div><div>- stack adjustment removed</div><div>- refactoring (but still in need of more)</div><div>- spill/restore insertion code unified with spill/restore placement code</div>
<div><br></div><div>Documentation available <a href="http://wiki.github.com/jdmdj/llvm-work/shrink-wrapping-work">here</a> illustrates shrink wrapping with loops and discusses a situation in which the pass would be more effective by splitting edges in the Machine CFG (similar to breaking crit. edges in the CFG).</div>
<div><br></div><div>Test cases are attached.</div><div><br></div><div>Thanks,</div><div>John</div><div><br></div>
<span><shrink-wrapping.P2.patch></span><span><test-sw.tar.gz></span>_______________________________________________<br>LLVM Developers mailing list<br><a href="mailto:LLVMdev@cs.uiuc.edu">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu">http://llvm.cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br></blockquote></div><br></div></body></html>