[llvm-commits] SSI patch

[Andre Tavares]

Nick Lewycky wrote:
> 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.
>
>   
I answered this on Eli's email. About "don't re-SSI the same variable", 
this is not possible, because different client passes could ask to 
transform the same variable, and one does not know which variables the 
other converted.
> 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
>>>   
>>>       
>
> _______________________________________________
> 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