[llvm-commits] SSI patch

[Andre Tavares]

Hey Nick, Thanks for your response, my comments are below.

Nick Lewycky wrote:
> 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 
> http://llvm.org/docs/ProgrammersManual.html 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.)
>
Changed them all from <vector> to SmallVector, and so on. I need 
SSI::created to have a record of what variables I have transformed to 
SSI, and if another pass asks to transform it, I will be able to not do 
anything because I already know it was transformed before. Without this 
I would have to try to transform it and see that it is not possible.


> 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.
>
Totally agree. Fixed them all.

> Some of your comments (SSI::insertPhiFunctions) are missing a space 
> after // and before the text. Please add it where missing.
>

Fixed.
>   // TODO: We do modify the programs code, but we do not know which
>   // Passes we break.
>   void SSI::getAnalysisUsage(AnalysisUsage &AU) const {
>     AU.addRequired<DominanceFrontier> ();
>     AU.addRequired<DominatorTree> ();
>     AU.setPreservesAll();
>   }
>
> 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().
>
>   bool SSI::runOnFunction(Function &F) {
>     DT_ = &getAnalysis<DominatorTree> ();
>     return false;
>   }
>
> 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".
>
>   /// Test if the BasicBlock BB dominates any use or definition of value.
>   ///
>   bool SSI::dominateAny(BasicBlock * BB, Instruction * value) {
>     Value::use_iterator begin = value->use_begin();
>     Value::use_iterator end = value->use_end();
>     for (; begin != end; ++begin) {
>       Instruction * I = dyn_cast<Instruction> (*begin);
>
> Use cast<Instruction> instead of dyn_cast<Instruction> since we know 
> in advance that it must be an Instruction.
>
>     BasicBlock * BB_father = I->getParent();
>     if (DT_->dominates(BB, BB_father)) {
>       return true;
>     }
>   }
>   return false;
> }

Fixed dyn_cast.
>
> 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.
>
Nick,  instead of moving only createSSI out of the pass, why don't we 
move SSI Pass to lib/Transforms/Utils/, and make it an utility class 
instead of a Pass? With this, we would not need to worry about 
getAnalysisUsage.

If we decide to make it an utility class, should I put all content in a 
.h file and forget about the .cpp?

> I'll try to do a more thorough review soon.
>
> Nick
>
> 2009/6/25 Andre Tavares <andrelct at dcc.ufmg.br 
> <mailto:andrelct at dcc.ufmg.br>>
>
>     Hello,
>
>     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.
>
>     // This pass converts a list of variables to the Static Single
>     Information
>     // form. This is a program representation described by Scott
>     Ananian in his
>     // Master Thesis: "The Static Single Information Form (1999)".
>     // We are building an on-demand representation, that is, we do not
>     convert
>     // every single variable in the target function to SSI form.
>     Rather, we receive
>     // a list of target variables that must be converted. We also do not
>     // completely convert a target variable to the SSI format.
>     Instead, we only
>     // change the variable in the points where new information can be
>     attached
>     // to its live range, that is, at branch points.
>
>     -- 
>     Andre Tavares
>     Master Student in Computer Science - UFMG - Brasil
>     http://dcc.ufmg.br/~andrelct <http://dcc.ufmg.br/%7Eandrelct>
>
>
>     _______________________________________________
>     llvm-commits mailing list
>     llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>     http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>   

-- 
Andre Tavares
Master Student in Computer Science - UFMG - Brasil
http://dcc.ufmg.br/~andrelct