[llvm-commits] SSI patch

[Nick Lewycky]

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

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.

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

  // 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;
}

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.

I'll try to do a more thorough review soon.

Nick

2009/6/25 Andre Tavares <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
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20090625/105698a9/attachment.html>