[llvm-commits] SSI patch

[Andre Tavares]

Nick Lewycky wrote:
> 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).
>   
ok
> 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.
>   
done
>    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.
>   
ok
>    // Insert phi functions if there is any sigma function
>    while (defsites[i].size() > 0) {
>
> Use "while (!defsites[i].empty()) {".
>   
ok
>    // 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],".
>   
ok
>    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)".
>   
ok. All those indenting problems are created by eclipse, I will indent 
all on hand now.
>    /// 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.
>   
I would never find that. Thanks
>    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....
>   
ok
>    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?
>   
I will do that. I was using And, just to find the files I'm creating 
easily, but I meant to remove it to send the patch.
> Please resubmit your changes as a unified diff next time so I can apply 
> them to my tree. Thanks!
>
> Nick
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>   

Nick thanks for help on this patch. And thanks every one else that 
helped on reviewing.

The patch is attached.

-- 
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-patch
Size: 35262 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20090703/d1ad145a/attachment.bin>