<div><div class="gmail_quote">I haven't looked closely at the algorithm yet. What jumped out were a number of stylistic concerns, first of all. We have a number of ADTs in LLVM that we like more than C++ STL ones (see <a href="http://llvm.org/docs/ProgrammersManual.html">http://llvm.org/docs/ProgrammersManual.html</a> for why). You should use SmallVector instead of std::vector&. For example, "SSI::createSSI(SmallVectorImpl<Instruction *> value) {", or possibly make it take an Instruction** and a size. Similarly, std::set is horrible in general, you should make SSI::created be a SmallPtrSet<Instruction*, 16>. (Or remove SSI::created entirely in favour of making createSSI take a set and tell the caller they're not allowed to call createSSI on the same value twice.)</div>
<div class="gmail_quote"><br></div><div class="gmail_quote">You declare some global constants which should at least also be static. You shouldn't evaluate X->size() on every iteration of a loop; for example the "for" statement in isUsedInTerminator should be written like "for (unsigned i = 0, e = TI->getNumOperands(); i != e; ++i) {". This is done in a few other places as well.</div>
<div class="gmail_quote"><br></div><div class="gmail_quote">Some of your comments (SSI::insertPhiFunctions) are missing a space after // and before the text. Please add it where missing.</div><div class="gmail_quote"><br>
</div><div class="gmail_quote"> // TODO: We do modify the programs code, but we do not know which<div> // Passes we break.</div><div> void SSI::getAnalysisUsage(AnalysisUsage &AU) const {</div><div> AU.addRequired<DominanceFrontier> ();</div>
<div> AU.addRequired<DominatorTree> ();</div><div> AU.setPreservesAll();</div><div> }</div><div><br></div><div>If you don't know what you preserve, don't preserve anything. I think your pass preserves the CFG (you don't modify any TerminatorInsts) so you can mark it AU.setPreservesCFG().</div>
<div><br></div><div> bool SSI::runOnFunction(Function &F) {</div><div> DT_ = &getAnalysis<DominatorTree> ();</div><div> return false;</div><div> }</div><div><br></div><div>This runOnFunction doesn't transform anything. That means that if I run "opt -ssi_and_pass" over a .bc then no transform or analysis will happen. That's okay -- opt -scalar-evolution does the same thing -- but if you do this you should also supply a print() method where the pass dumps out what it would have done (the results of its analysis on every variable, in scalar-evolutions case). You can then read it with "opt -analyze -ssi_and_pass".</div>
<div><br></div><div> /// Test if the BasicBlock BB dominates any use or definition of value.</div><div> ///</div><div> bool SSI::dominateAny(BasicBlock * BB, Instruction * value) {</div><div> Value::use_iterator begin = value->use_begin();</div>
<div> Value::use_iterator end = value->use_end();</div><div> for (; begin != end; ++begin) {</div><div> Instruction * I = dyn_cast<Instruction> (*begin);</div><div><br></div><div>Use cast<Instruction> instead of dyn_cast<Instruction> since we know in advance that it must be an Instruction.</div>
<div><br></div><div> BasicBlock * BB_father = I->getParent();</div><div> if (DT_->dominates(BB, BB_father)) {</div><div> return true;</div><div> }</div><div> }</div><div> return false;</div><div>}</div>
<div><br></div><div>So overall this looks like a good start. I think we may want to move createSSI out of the pass and into its own lib/Transforms/Utils/SSI.cpp which takes the DominatorTree* and DominatorFrontier*. This method could then be called from any pass without having to declare it in advance in its own analysis usage. Then the SSI pass would then exist for testing purposes; it could do something like either collect the sensible instructions or maybe just all non-void instructions and run createSSI over them.</div>
<div><br></div><div>I'll try to do a more thorough review soon.</div><div><br></div><div>Nick</div></div><div class="gmail_quote"><br></div><div class="gmail_quote">2009/6/25 Andre Tavares <span dir="ltr"><<a href="mailto:andrelct@dcc.ufmg.br">andrelct@dcc.ufmg.br</a>></span><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">Hello,<br>
<br>
I'm submiting a patch that will be responsible to create SSI representation on demand. This representation conforms with the current SSA, and will not break other optimizations. It only inserts some phi functions on the code. A brief description is below, and the code is attached.<br>
<br>
// This pass converts a list of variables to the Static Single Information<br>
// form. This is a program representation described by Scott Ananian in his<br>
// Master Thesis: "The Static Single Information Form (1999)".<br>
// We are building an on-demand representation, that is, we do not convert<br>
// every single variable in the target function to SSI form. Rather, we receive<br>
// a list of target variables that must be converted. We also do not<br>
// completely convert a target variable to the SSI format. Instead, we only<br>
// change the variable in the points where new information can be attached<br>
// to its live range, that is, at branch points.<br><font color="#888888">
<br>
-- <br>
Andre Tavares<br>
Master Student in Computer Science - UFMG - Brasil<br>
<a href="http://dcc.ufmg.br/~andrelct" target="_blank">http://dcc.ufmg.br/~andrelct</a><br>
<br>
</font><br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>