[llvm-commits] [LLVMdev] SSI Patch
Andre Tavares
andrelct at dcc.ufmg.br
Thu Sep 17 07:36:07 PDT 2009
I made all changes. Hope it is good now.
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
>
>
>
--
Andre Tavares
Master Student in Computer Science - UFMG - Brasil
http://dcc.ufmg.br/~andrelct
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ssi.patch
Type: text/x-diff
Size: 21346 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20090917/d7344f80/attachment.patch>
More information about the llvm-commits
mailing list