[llvm-commits] SSI patch

[Nick Lewycky]

Andre Tavares wrote:
> 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.

   // Test if there is a need to transform to SSI
   if (needConstruction.count() > 0) {

Use !needConstruction.empty() .

   void SSI::insertSigmaFunctions(std::vector<Instruction *> & value) {
     for (unsigned i = 0; i < num_values; ++i) {
       if (needConstruction[i]) {

Just put "if (!needConstruction[i]) continue;" to avoid indenting 
everything another level.

   for (; begin != end; ++begin) {
     // Test if the Use of the Value is in a comparator
     CmpInst * CI;
     if ((CI = dyn_cast<CmpInst> (begin)) && isUsedInTerminator(CI)) {

How about:
   for (; begin != end; ++begin) {
     CmpInst *CI = dyn_cast<CmpInst>(begin);
     if (!CI || !isUsedInTerminator(CI)) {
instead?

In insertPhiFunctions:

   std::set<BasicBlock *> * BB_visited = new std::set<BasicBlock *>();
   while (...) { ...
     free(BB_visited);
   }

How about allocating std::set<BasicBlock *> BB_visited on the stack and 
calling BB_visited.clear() each iteration through the loop?

   // Test if has not yet visited this node and if the
   // original definition dominates this node
   if (BB_visited->insert(BB_dominated).second && value_original[i]
       != BB_dominated
       && DT_->dominates(value_original[i], BB_dominated)
       && dominateAny(BB_dominated, value[i])) {

I think the "value_original[i] != BB_dominated &&
DT_->dominates(value_original[i], BB_dominated)" part can be rewritten 
as "DT_->properlyDominates(value_original[i], BB_dominated)".

Also, please fit the operators on the end of the line.

In SSI::rename:
   // For SSI_SIG (b = PHI(a)), b is a new definition to a. So we need to
"new definition *of a"?
   // Then store bin the value_stack as the new definition of a.
Not sure what the means.

   TerminatorInst * TI = BB->getTerminator();
   for (unsigned i = 0; i < TI->getNumSuccessors(); ++i) {
     BasicBlock * BB_succ = TI->getSuccessor(i);

Perhaps you wanted:
   for (succ_iterator SI = succ_begin(BI), SE = succ_end(B); SI != SE; 
++SI) {
? You need to #include "llvm/Support/CFG.h" for that.

   // This loop calls rename on all children from this block. This time 
children
   // refers to a successor block in the dominance tree.

In other words, it calls rename on all immediately dominated blocks?

   if (defined->count() > 0) {

Use "if (defined->any()) {". Count isn't particularly expensive but 
any() is cheaper.


   void SSI::substituteUse(Instruction * I) {
     for (unsigned i = 0; i < I->getNumOperands(); ++i) {
       Value * operand = I->getOperand(i);
       for (unsigned j = 0; j < num_values; ++j) {
         if (operand == value_stack[j].front() && I != 
value_stack[j].back()) {

It's not clear to me what the if-statement is actually doing, and 
whether it's any different from just using "if (i != j) continue;".

   if ((I_phi = dyn_cast<PHINode> (I)) && (vs_phi = dyn_cast<PHINode> (
       value_stack[j].back())) && value_stack[j].back()->getParent()
       == I->getParent()) {

This needs to be line-flowed better. Here's my suggestion:
   if ((I_phi = dyn_cast<PHINode>(I)) &&
       (vs_phi = dyn_cast<PHINode> (value_stack[j].back())) &&
       value_stack[j].back()->getParent() == I->getParent()) {
which keeps each 'phrase' on one line.

   /// Test if the BasicBlock BB dominates any use or definition of value.
   ///
   bool SSI::dominateAny(BasicBlock * BB, Instruction * value) {

This whole method should work by a) checking whether the Instruction 
itself is dominator (if so, so are all non-phi uses, so return true 
fast) else just loop over the users who are PHINodes and test them only.

Break SSI::getPosition up into getPositionPhi and getPositionSigma. All 
callers put a constant value into compare anyways, and there's no way 
that copying an std::map like that can be cheap.

In SSI::isUsedInTerminator, we know that the comparison field of a 
BranchInst or SwitchInst is going to be field 0. Just make sure 
getNumOperands() != 0 then look at field 0. No loop needed.


Overall, looks good! I like your choice of Ananian's algorithm over 
Singer's because it avoids requiring reverse dominfo (yay!). I didn't 
see anything wrong with the algorithm details.

The one thing to remember is that when placing sigma's in the first 
place you use a very simple flow of "is it a CmpInst which flows to a 
TerminatorInst". That's probably good enough for ABCD but not for a 
GCC-like VRP algorithm. You don't need to worry about that though, 
whoever tried to implement a GCC-like VRP gets to worry about it.

Nick