[llvm-commits] [LLVMdev] SSI Patch

[Nick Lewycky]

Andre Tavares wrote:
> I made all changes. Hope it is good now.

       // Treat SSI_PHI
       if (Variable *var = getVarFromPhi(PN)) {
         var->addValueStack(PN);
         defined.insert(var);
       }

       // Treat SSI_SIG
       else if (Variable *var = getVarFromPhi(PN)) {
         substituteUse(I);
         var->addValueStack(PN);
         defined.insert(var);
       }

Those are both testing getVarFromPhi. Should the second one be 
getVarFromSigma instead?

And since we have to make one change, we may as well make others :)

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

Whitespace.

     if (PHINode *PN_inc = dyn_cast<PHINode>(inc)) {
       if (isSigPhi(PN_inc, I, Set))
         continue;
       return false;
     }
     return false;
   }

Isn't that just:

     Value *PN_inc = dyn_cast<PHINode>(inc);
     if (!PN_inc || !isSigPhi(PN_inc, I, Set))
       return false;
   }

given that the final } is closing the loop?

Finally, 'isSigPhi' is an ambiguous name. Perhaps it should be named 
'isSigma'?

Nick

> Nick Lewycky wrote:
>> 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
>>
>>
>>
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits