[llvm-commits] SSI patch

[Nick Lewycky]

Andre Tavares wrote:
> Latest version of the code, after all considerations.

A couple notes about style. You seem to write things like:

   a) PHINode * phi;
   b) if (I_phi && vs_phi && value_stack[j].back()->getParent()
          == I->getParent()) {

while most LLVM code is written:

   a) PHINode *PN;
   b) if (I_phi && vs_phi &&
          value_stack[j].back()->getParent() == I->getParent()) {

The old hard-and-fast rule is that you're consistent within the file, 
and you are, but unless you feel very strongly about this I'd like you 
to keep the style similar to that of the rest of LLVM's existing code.

For a) that's the placement of the * and to a lesser extent the variable 
naming and for b) that's putting the operator before the line break 
instead of starting a line with an operator (also in for loops).

SSI_And.cpp:

   const std::string SSI_PHI = "SSI_phi";
   const std::string SSI_SIG = "SSI_sigma";

   const unsigned UNSIGNED_INFINITE = ~0U;

Mark those three static.

   PHINode * phi = PHINode::Create(value[i]->getType(),
       SSI_SIG, BB_next->begin());

The line with SSI_SIG has to line up with value[i].... Alternately, you 
can place all three arguments on the next line indented by 4 spaces. 
This shows up again in insertPhiFunctions but with BB_dominated instead 
of BB_next.

   // Insert phi functions if there is any sigma function
   while (defsites[i].size() > 0) {

Use "while (!defsites[i].empty()) {".

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

The line with just "BB_dominated" is indented wrong, please make it in 
line with "value_original[i],".

   PHINode * I_phi = dyn_cast<PHINode> (I);
   PHINode * vs_phi = dyn_cast<PHINode> (value_stack[j].back());

There's no space in "dyn_cast<PHINode>(I)".

   /// Test if the BasicBlock BB dominates any use or definition of value.

Andre Tavares wrote:
>>    /// 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.
>> >
>> >   
> Don't get what you mean here. I have to test all users of the 
> instruction not only the PHINodes, correct?
> This method is testing if the value is alive in BB. As there is no 
> LiveVariableAnalysis here I used Dominance.

Oh I see. You're checking whether a given BB dominates any user of the 
value. That's even what's you said. :) Never mind!

   ///
   /// When there is a phi node that is created in a BasicBlock and it 
is used
   /// as an operand of another phi function used in the same BasicBlock,
   /// LLVM looks this as an error. So on the second phi, the first phi 
is called
   /// P and the BasicBlock it incomes is B. This P will be replaced by 
the value
   /// it has for BasicBlock B.
   void SSI::fixPhis() {

The blank /// is on the top of the comment while the other in the file 
put it at the bottom.

   void SSI::fixPhis() {
     for (SmallPtrSet<PHINode *, 1>::iterator begin = phisToFix.begin(),
         end = phisToFix.end(); begin != end; ++begin) {

Need to indent the line beginning with "end" by 1 space so it lines up 
with SmallPtrSet....

   static RegisterPass<SSI> SSIAND("ssi_and_pass", "SSI_And Pass");

Why "ssi_and"? Why not just:
   static RegisterPass<SSI> X("ssi", "SSI Construction");
and name the files SSI.cpp and SSI.h?

Please resubmit your changes as a unified diff next time so I can apply 
them to my tree. Thanks!

Nick