[PATCH] D35474: SSAUpdater: Add mode when Phi creation is not allowed

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

In https://reviews.llvm.org/D35474#812370, @skatkov wrote:

> Hi Daniel, I plan to upload the patch for PR26233 this week. I'd like to think a bit more about relation of recursion and the patch I have - whether it works correctly and whether SSAUpdater work correctly with this pattern.
>
> For now, shortly, the idea of future patch is as follows: in CodeGenPrepare::OptimizeMemoryInst I'd like to collect all cases where address computation is identical but base might be different. So to sink address computation to memory instruction I need to find a common Phi node for all bases which can be used in BB where memory instruction is located. For that purpose I'd like to use SSAUpdater. However it is possible that Phi node for bases may not exists (for example if there is no further uses of base it can be removed).


Please don't use SSA updater for this.
This is trivial to do without it., and SSAUpdater can't do it any faster than you can.

> It is not clear that creation of new Phi node is profitable with respect to folding of address computation to memory instruction.
>  I'd like to add an option which allows/disallows creation of Phi nodes. For that I need this patch.

I'm really strongly opposed to doing it for this use case.

> SSAUpdater is able to correctly answer to the question: if I know the different values in different basic blocks is there any merge of these values in BB I'm interested in. In general it is not so easy and straightforward in complex CFG.

I strongly disagree. 
The algorithm is < 100 lines of code.
SSAUpdater is not going to perform any magic here.

The algorithm you are looking for is a simple walking algorithm that looks like this (taken from a paper i can give you if you like).

(The code below supports tracking multiple variables at once, you could remove unsigned Var if you only ever are trying one at a time)

CurrentDef is a map from std::pair{varnum, bb} to a definition of varnum in BB.
Fill it in with your existing values.

  void writeVariable(unsigned Var, BasicBlock *BB, Value *V) {
    CurrentDef[{Var, BB}] = V;
  }
  Value *GetExistingVariableOrCreate(unsigned Var, BasicBlock *BB) {
    auto CDI = CurrentDefs.find({Var, BB});
    if (CDI != CurrentDefs.end())
      return CDI->second;
    return GetVariableInBlock(Var, BB);
  }
  Value *GetVariableInBlock(unsigned int VarNum, BasicBlock *BB)
  {
    Value *Result;
  
    // Single predecessor case, just recurse
    if (BasicBlock *Pred = BB->getSinglePredecessor()) {
      Result = GetVariableInBlock(Var, Pred);
    } else if (VisitedBlocks.count({Var, BB})) {
      // We hit our node again, meaning we had a cycle, we must insert a phi
      // node to break it.
     Result = Create an empty phi we'll fill in later.
      {Search for PHI Node here, you need one, if you don't find one, you need a phi but don't have one}
    } else {
      // Mark us visited so we can detect a cycle
      VisitedBlocks.insert({Var, BB});
      SmallVector<Value *, 8> PHIOps;
  
      // Recurse to get the values in our predecessors. This will look for phi nodes if we cycle.
      for (auto *Pred : predecessors(BB))
        PHIOps.push_back(GetVariableInBlock(Var, Pred));
        Result = {Search for PHI that has PHIOps in BB}
  
   writeVariable(Var, BB, Result);
   return Result;
  }

Using the current SSAUpdater to do this will compute dominance frontiers, etc, all which is a complete waste of time for you.
You are just trying to see if there are merge blocks that values reach.

The above is one way of doing it (by walking up).

If you are only concerned about existing phis, you can also just look at the uses.


https://reviews.llvm.org/D35474