[llvm-commits] SSI patch

[Andre Tavares]

Nick Lewycky wrote:
> Andre Tavares wrote:
>   
>> Hello,
>>
>> I'm submiting a patch that will be responsible to create SSI 
>> representation on demand. This representation conforms with the current 
>> SSA, and will not break other optimizations. It only inserts some phi 
>> functions on the code. A brief description is below, and the code is 
>> attached.
>>
>> // This pass converts a list of variables to the Static Single Information
>> // form. This is a program representation described by Scott Ananian in his
>> // Master Thesis: "The Static Single Information Form (1999)".
>> // We are building an on-demand representation, that is, we do not convert
>> // every single variable in the target function to SSI form. Rather, we 
>> receive
>> // a list of target variables that must be converted. We also do not
>> // completely convert a target variable to the SSI format. Instead, we only
>> // change the variable in the points where new information can be attached
>> // to its live range, that is, at branch points.
>>     
>
>    // Test if there is a need to transform to SSI
>    if (needConstruction.count() > 0) {
>
> Use !needConstruction.empty() .
>   
Yep, changed to any, as you mentioned below somewhere.

>    void SSI::insertSigmaFunctions(std::vector<Instruction *> & value) {
>      for (unsigned i = 0; i < num_values; ++i) {
>        if (needConstruction[i]) {
>
> Just put "if (!needConstruction[i]) continue;" to avoid indenting 
> everything another level.
>   
ok

>    for (; begin != end; ++begin) {
>      // Test if the Use of the Value is in a comparator
>      CmpInst * CI;
>      if ((CI = dyn_cast<CmpInst> (begin)) && isUsedInTerminator(CI)) {
>
> How about:
>    for (; begin != end; ++begin) {
>      CmpInst *CI = dyn_cast<CmpInst>(begin);
>      if (!CI || !isUsedInTerminator(CI)) {
> instead?
>   
ok. Isn't it without !. Like

if (CI || !isUsedInTerminator(CI)) {

> In insertPhiFunctions:
>
>    std::set<BasicBlock *> * BB_visited = new std::set<BasicBlock *>();
>    while (...) { ...
>      free(BB_visited);
>    }
>
> How about allocating std::set<BasicBlock *> BB_visited on the stack and 
> calling BB_visited.clear() each iteration through the loop?
>
>   
Done already, just didn't send the latest version. Thanks.

>    // Test if has not yet visited this node and if the
>    // original definition dominates this node
>    if (BB_visited->insert(BB_dominated).second && value_original[i]
>        != BB_dominated
>        && DT_->dominates(value_original[i], BB_dominated)
>        && dominateAny(BB_dominated, value[i])) {
>
> I think the "value_original[i] != BB_dominated &&
> DT_->dominates(value_original[i], BB_dominated)" part can be rewritten 
> as "DT_->properlyDominates(value_original[i], BB_dominated)".
>
>   
Of coarse. Missed that.
> Also, please fit the operators on the end of the line.
>
> In SSI::rename:
>    // For SSI_SIG (b = PHI(a)), b is a new definition to a. So we need to
> "new definition *of a"?
>    // Then store bin the value_stack as the new definition of a.
> Not sure what the means.
>   
I mean that b is a sigma of a, so it is a new definition of a. But I 
changed that part to look better.
>    TerminatorInst * TI = BB->getTerminator();
>    for (unsigned i = 0; i < TI->getNumSuccessors(); ++i) {
>      BasicBlock * BB_succ = TI->getSuccessor(i);
>
> Perhaps you wanted:
>    for (succ_iterator SI = succ_begin(BI), SE = succ_end(B); SI != SE; 
> ++SI) {
> ? You need to #include "llvm/Support/CFG.h" for that.
>
>   
ok.
>    // This loop calls rename on all children from this block. This time 
> children
>    // refers to a successor block in the dominance tree.
>
> In other words, it calls rename on all immediately dominated blocks?
>
>   
yes.
>    if (defined->count() > 0) {
>
> Use "if (defined->any()) {". Count isn't particularly expensive but 
> any() is cheaper.
>
>
>   
ok
>    void SSI::substituteUse(Instruction * I) {
>      for (unsigned i = 0; i < I->getNumOperands(); ++i) {
>        Value * operand = I->getOperand(i);
>        for (unsigned j = 0; j < num_values; ++j) {
>          if (operand == value_stack[j].front() && I != 
> value_stack[j].back()) {
>
> It's not clear to me what the if-statement is actually doing, and 
> whether it's any different from just using "if (i != j) continue;".
>   
There are two for here. The outside for iterates in all operands of I, 
and the inside in all variables I received. The first part of the if is 
testing if the operand of the moment is the variable of the moment. I 
can't use "if (i != j) continue;", because i and j are used to iterate 
in different things. Was clear?
>    if ((I_phi = dyn_cast<PHINode> (I)) && (vs_phi = dyn_cast<PHINode> (
>        value_stack[j].back())) && value_stack[j].back()->getParent()
>        == I->getParent()) {
>
> This needs to be line-flowed better. Here's my suggestion:
>    if ((I_phi = dyn_cast<PHINode>(I)) &&
>        (vs_phi = dyn_cast<PHINode> (value_stack[j].back())) &&
>        value_stack[j].back()->getParent() == I->getParent()) {
> which keeps each 'phrase' on one line.
>   

Changed to
PHINode * I_phi = dyn_cast<PHINode> (I);
PHINode * vs_phi = dyn_cast<PHINode> (value_stack[j].back());
if (I_phi && vs_phi && value_stack[j].back()->getParent()
== I->getParent()) {
>    /// 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.

> Break SSI::getPosition up into getPositionPhi and getPositionSigma. All 
> callers put a constant value into compare anyways, and there's no way 
> that copying an std::map like that can be cheap.
>
>   
ok
> In SSI::isUsedInTerminator, we know that the comparison field of a 
> BranchInst or SwitchInst is going to be field 0. Just make sure 
> getNumOperands() != 0 then look at field 0. No loop needed.
>
>   
great.
> Overall, looks good! I like your choice of Ananian's algorithm over 
> Singer's because it avoids requiring reverse dominfo (yay!). I didn't 
> see anything wrong with the algorithm details.
>
> The one thing to remember is that when placing sigma's in the first 
> place you use a very simple flow of "is it a CmpInst which flows to a 
> TerminatorInst". That's probably good enough for ABCD but not for a 
> GCC-like VRP algorithm. You don't need to worry about that though, 
> whoever tried to implement a GCC-like VRP gets to worry about it.
>
> Nick
> _______________________________________________
> 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