[PATCH] Add control dependence computation that computes control dependence equivalence classes.

[Daniel Berlin]

I'll also rename it to ControlDependenceEquivalence.


On Tue, Apr 14, 2015 at 7:41 AM, Chandler Carruth <chandlerc at gmail.com> wrote:
> I can't comment on the algorithmic side, but am happy to review the basic code.
>
> First comment would be that if this is really only providing equivalence classes, it should be named appropriately. Probably ControlDependenceEquivalence or some such. Do that last though so the in-line comments aren't lost.
>
>
> ================
> Comment at: include/llvm/Analysis/ControlDependence.h:9-12
> @@ +8,6 @@
> +//===----------------------------------------------------------------------===//
> +// \file
> +// \brief Determines control dependence equivalence classes for basic
> +// blocks. Any two blocks having the same set of control dependences land in one
> +// class.
> +// ===---------------------------------------------------------------------===//
> ----------------
> You need three '/'s for the doxygen stuff.
>
> ================
> Comment at: include/llvm/Analysis/ControlDependence.h:59-64
> @@ +58,8 @@
> +
> +  /// \brief Return the control dependence class number for \p BB.  All basic
> +  /// blocks with the same class number have the same control dependences
> +  unsigned getClassNumber(BasicBlock *BB) {
> +    assert(Computed && "Trying to get equivalence classes before computation");
> +    return BlockInfo[BB].ClassNumber;
> +  }
> +
> ----------------
> Is 'unsigned' enough? (I can believe it is, I just want to surface the question.)
>
> ================
> Comment at: lib/Analysis/ControlDependence.cpp:21-24
> @@ +20,6 @@
> +using namespace llvm;
> +char ControlDependence::ID = 0;
> +#define DEBUG_TYPE "controlequiv"
> +INITIALIZE_PASS(ControlDependence, "controldep",
> +                "Control Dependence Construction", true, true);
> +
> ----------------
> Why are these jammed in here with headers and usings?
>
> ================
> Comment at: lib/Analysis/ControlDependence.cpp:29
> @@ +28,3 @@
> +// numbers for all participating blocks. Takes O(E) time and O(N) space.
> +
> +bool ControlDependence::runOnFunction(Function &F) {
> ----------------
> Weird whitspace...
>
> ================
> Comment at: lib/Analysis/ControlDependence.cpp:122-134
> @@ +121,15 @@
> +        // and preorder-visit
> +        if (PredData.OnStack) {
> +          DEBUG(dbgs() << "Maybe visit pred backedge\n");
> +          if (Pred != Entry.ParentBlock) {
> +            BlockInfo[B].Backedges.push_back(Pred);
> +            visitBackedge(B, Pred, PredDirection);
> +          }
> +
> +        } else {
> +          BlockInfo[B].Children.push_back(Pred);
> +          pushDFS(Stack, Pred, B, PredDirection);
> +          visitPre(Pred);
> +        }
> +        continue;
> +      }
> ----------------
> This looks like it could be cleaned up with early-continue patterns.
>
> ================
> Comment at: lib/Analysis/ControlDependence.cpp:200-201
> @@ +199,4 @@
> +      combined_succ_end(B, BlockResultData.FakeSuccEdges), From, B});
> +}
> +/// \brief Pop an entry from the DFS stack.
> +void ControlDependence::popDFS(DFSStack &Stack, const BasicBlock *B) {
> ----------------
> Missing vertical whitespace.
>
> ================
> Comment at: lib/Analysis/ControlDependence.cpp:260-261
> @@ +259,4 @@
> +      BII = Info.BracketIterators.erase(BII);
> +    } else
> +      ++BII;
> +  }
> ----------------
> Keep braces symmetric.
>
> ================
> Comment at: lib/Analysis/ControlDependence.cpp:311-321
> @@ +310,13 @@
> +    else {
> +      // Get the min and the max through our children
> +      auto hi1iter = std::min_element(
> +          Info.Children.begin(), Info.Children.end(),
> +          [this](const BasicBlock *A, const BasicBlock *B) {
> +            return BlockInfo[A].DFSNumber < BlockInfo[B].DFSNumber;
> +          });
> +      auto hi2iter = std::max_element(
> +          Info.Children.begin(), Info.Children.end(),
> +          [this](const BasicBlock *A, const BasicBlock *B) {
> +            return BlockInfo[A].DFSNumber < BlockInfo[B].DFSNumber;
> +          });
> +      Hi1 = *hi1iter;
> ----------------
> Variable naming: Hi1Iter
>
> ================
> Comment at: lib/Analysis/ControlDependence.cpp:376-377
> @@ +375,4 @@
> +  DEBUG(dbgs() << "Assigned class number is " << Info.ClassNumber << "\n");
> +}
> +void ControlDependence::visitPost(const BasicBlock *B,
> +                                  const BasicBlock *ParentBlock,
> ----------------
> Again, missing whitespace.
>
> ================
> Comment at: lib/Analysis/ControlDependence.cpp:390-402
> @@ +389,15 @@
> +
> +  // By the time we hit this node, we are guaranteed these iterators will point
> +  // into our list, because it must have been spliced into us.
> +  for (auto BII = Info.BracketIterators.begin(),
> +            BIE = Info.BracketIterators.end();
> +       BII != BIE;) {
> +    BracketList::iterator &BLI = *BII;
> +    Bracket &BR = *BLI;
> +    if (BR.To == B && BR.Direction != Direction) {
> +      Info.BList.erase(BLI);
> +      BII = Info.BracketIterators.erase(BII);
> +    } else
> +      ++BII;
> +  }
> +
> ----------------
> This code seems completely duplicated with the above?
>
> ================
> Comment at: lib/Analysis/ControlDependence.cpp:410
> @@ +409,3 @@
> +    BracketList &ParentBList = BlockInfo[ParentBlock].BList;
> +    // TODO
> +    ParentBList.splice(ParentBList.end(), BList);
> ----------------
> TODO what?
>
> ================
> Comment at: lib/Analysis/ControlDependence.cpp:433-434
> @@ +432,4 @@
> +  BlockInfo[To].BracketIterators.push_back(Place);
> +}
> +FunctionPass *llvm::createControlDependencePass() {
> +  return new ControlDependence();
> ----------------
> Missing whitespace.
>
> http://reviews.llvm.org/D8568
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
>
>