[llvm-commits] SSI patch

[Nick Lewycky]

Go editing! I have a couple corrections to make to myself:

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:

"You seem to write" -> "You've written"

> 
>    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, 

"The old" -> "The only"

Nick

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