[llvm-commits] SSI patch
Andre Tavares
andrelct at dcc.ufmg.br
Mon Jun 29 12:42:13 PDT 2009
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
More information about the llvm-commits
mailing list