[llvm-commits] [LLVMdev] SSI Patch

[Nick Lewycky]

Andre Tavares wrote:
> Next step:
> 
> There was that list of all variables converted to SSI. I decided to 
> remove it as you guys didn't like to have list holding memory. Now 
> insertSigma tests if a sigma is already there before it inserts any.

+  for (; begin != notphi; ++begin) {
+    PHINode *PN = cast<PHINode>(begin);
+    SmallPtrSet<PHINode *, 16> *Set = new SmallPtrSet<PHINode *, 16>();
+    if (isSigPhi(PN, I, Set)) {
+      delete(Set);
+      return PN;
      }
    }

Please don't do that. The point of SmallSet/SmallVector is that they 
allocate the number of entries in the template parameter on the stack so 
as to avoid extra malloc traffic. 'new SmallPtrSet' defeats that 
entirely. You can simply declare it on the stack before the loop and 
clear it in between each run of the loop with Set.clear();.

Same thing again in findPhi().

+  // BB has one predecessor, BB cannot have phis.
+  if (BB->getSinglePredecessor() != NULL || I == NULL)
+    return NULL;

Do you expect callers to pass NULL in as the Instruction? If not, you 
could use an assert that I is not NULL.

+      delete(Set);

You don't need the parenthesis.

+      if ((var = getVarFromPhi(PN)) != NULL) {

You don't need the "!= NULL" part, it's implicit. Also, since you're not 
using var outside this if, please declare it in the expression like so:

   if (Variable *var = getVarFromPhi(PN)) {

Same thing again for SSI_SIG right below it.

+/// Test if PN is a Sigma or Phi from instruction I. This function returns
+/// true if all operands of PN lead to I. Its is necessary to keep a set of
+/// visited PNs (Set), in case there is a loop in Sigma and Phi functions.
+///

"Its necessary" -> It's necessary

+bool SSI::isSigPhi(PHINode *PN, Instruction *I,
+    SmallPtrSet<PHINode *, 16> *Set) {

Not a huge deal, but Set could be a reference here. Alternately I think 
you could use "SmallPtrImpl<PHINode *> *Set" which would remove the 
restatement of the 16. I not sure whether you can do both changes though.

Please indent 'SmallPtrSet<...' to line up with the 'PHINode *PN...' 
above it.

+  Set->insert(PN);

You want to return if it's already in the set, right? That makes it "if 
(!Set->insert(PN)) return ?;".

+  for (unsigned i=0, e=PN->getNumIncomingValues(); i<e ; ++i) {

Please put spaces around your = and < signs, and no space before semicolons.

+    Value *inc = PN->getIncomingValue(i);
+    if (inc == I)
+      continue;
+    else if (PHINode *PN_inc = dyn_cast<PHINode>(inc)) {
+      if ((Set->count(PN_inc) || isSigPhi(PN_inc, I, Set)))
+        continue;
+      else
+        return false;
+    } else
+      return false;
+  }

If you have an if-statement which causes control-flow to move elsewhere 
(like "return" or "continue") then don't follow it up with an else-if. 
Just write another if statement afterwards. 
http://llvm.org/docs/CodingStandards.html#hl_else_after_return

+SSI::Variable * SSI::getVarFromPhi(PHINode *PN) {

No space after the *

+  DenseMap<PHINode *, Variable *>::iterator val = phis.find(PN);
    if (val == phis.end())
-    return UNSIGNED_INFINITE;
+    return NULL;
    else
      return val->second;

Again, same problem: no else after return. (Yes, I realize it was there 
before your patch. Please fix it anyways.)

+void SSI::init(SmallPtrSet<Instruction *, 16> &v) {
+  SmallPtrSetIterator<Instruction *> begin = v.begin();
+  SmallPtrSetIterator<Instruction *> end = v.end();
+  for (; begin != end; ++begin) {
+    Instruction *I= *begin;

Spaces around the = sign.

+    variables.push_back(new Variable(I));
    }

  /// Clean all used resources in this creation of SSI
  ///
  void SSI::clean() {
-  for (unsigned i = 0; i < num_values; ++i) {
-    defsites[i].clear();
-    if (i < value_stack.size())
-      value_stack[i].clear();
+  for (unsigned i = 0; i < variables.size(); ++i) {
+    variables[i]->clear();
+    delete variables[i];
    }

You don't need to call clear() on a Variable before deleting it. They 
don't hold any pointers so they'll clean up cleanly during normal 
destruction.

+          Variable(Instruction *v) {
+            value = v;
+            need = false;
+            def_sites.push_back(value->getParent());
+            value_stack.push_back(value);
+          }

Please use the initializer syntax for 'value' and 'need'.

+          bool needConstruction() {return need;}
+          Instruction *getValue() {return value;}
+          unsigned defSitesSize() {return def_sites.size();}
[...]

Needs spaces around braces.

+      bool isSigPhi(PHINode *PN, Instruction *I,
+          SmallPtrSet<PHINode *, 16> *Set);

Again, indentation of SmallPtrSet should line up with PHINode.

+      bool hasDefinition(Variable *var, BasicBlock *BB);

bool isDefinedIn(), maybe?

+      Variable *getVarFromPhi(PHINode *PN);
+      Variable *getVarFromSigma(PHINode *PN);

Nick

> Andre Tavares wrote:
>> Missed that. Thanks.
>>
>> I will submit the next one today.
>>
>> Nick Lewycky wrote:
>>  
>>> Andre Tavares wrote:
>>>    
>>>> It is in its method now.
>>>>       
>>> I noticed that insertSigma() still used 'unsigned j' when it didn't 
>>> have to (it used to use 'j' instead of 'i' because 'i' was used in an 
>>> outer loop). I changed that and committed it. Thanks!
>>>
>>> Nick
>>>
>>>    
>>>> Nick Lewycky wrote:
>>>>      
>>>>> Andre Tavares wrote:
>>>>>        
>>>>>> I tried to make 5 separate patches, but as they are constructive, 
>>>>>> they had information from the last one. So I will post one by one 
>>>>>> as it gets on the tree.
>>>>>>
>>>>>> 1. We had a function isUsedInTerminator that tested if a 
>>>>>> comparator was used in the terminator of its parent BasicBlock. 
>>>>>> This is wrong because a comparator can be created in a BasicBlock 
>>>>>> and used in the terminator of other BasicBlock, and can be used in 
>>>>>> more than one.
>>>>>>           
>>>>> +            for (unsigned j = 0, e = TI->getNumSuccessors(); j < 
>>>>> e; ++j) {
>>>>> +              // Next Basic Block
>>>>> +              BasicBlock *BB_next = TI->getSuccessor(j);
>>>>> +              if (BB_next != BB &&
>>>>> +                  BB_next->getSinglePredecessor() != NULL &&
>>>>> +                  dominateAny(BB_next, value[i])) {
>>>>> +                PHINode *PN = PHINode::Create(
>>>>> +                    value[i]->getType(), SSI_SIG, BB_next->begin());
>>>>> +                PN->addIncoming(value[i], BB);
>>>>> +                sigmas.insert(std::make_pair(PN, i));
>>>>> +                created.insert(PN);
>>>>> +                need = true;
>>>>> +                defsites[i].push_back(BB_next);
>>>>> +                ++NumSigmaInserted;
>>>>> +              }
>>>>> +            }
>>>>>
>>>>> Please break this out into its own method. Otherwise, this looks 
>>>>> fine to me.
>>>>>
>>>>> Nick
>>>>>
>>>>>         
>>>> ------------------------------------------------------------------------ 
>>>>
>>>>
>>>> _______________________________________________
>>>> 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