[llvm-commits] SSI patch

[Nick Lewycky]

Andre Tavares wrote:
> 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'm wondering whether we need it to be a class or if it can just be a 
public method in the llvm:: namespace. The question there is whether it 
has state that it needs to keep between runs, and as far as I can see 
the answer is no.

The one piece of state that's currently kept is SSI::created. How bad is 
it if you're asked to create SSI on a variable that already has it? Is 
it computationally expensive or a recipe for miscompiles and crashes? 
Any reason we couldn't just make "don't re-SSI the same variable" the 
caller's concern? Don't forget that the caller is IMHO likely to already 
be keeping track of which variables have had SSI run on them making 
SSI::created redundant.

Nick

>> 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
>>   
>