[llvm-commits] [llvm] r122719 - in /llvm/trunk: include/llvm/InitializePasses.h include/llvm/Transforms/Scalar.h lib/Transforms/Scalar/CMakeLists.txt lib/Transforms/Scalar/LoopInstSimplify.cpp lib/Transforms/Scalar/Scalar.cpp

[Duncan Sands]

Hi Cameron,

> Add a new loop-instsimplify pass, with the intention of replacing the instance
> of instcombine that is currently in the middle of the loop pass pipeline. This
> commit only checks in the pass; it will hopefully be enabled by default later.

...

> +      AU.addRequired<DominatorTree>();

Why do you require the dominator tree?  Starting from the header you can get all
CFG successors by looping over the successors and visiting those.  By the very
definition of dominance this will visit definitions before uses.  It is simple
to check whether a basic block is in the loop using LoopInfo, so you can just
skip the ones that aren't.  I guess there might be a problem with subloops if
you want to skip those.

> +bool LoopInstSimplify::runOnLoop(Loop* L, LPPassManager&  LPM) {
> +  DominatorTree* DT =&getAnalysis<DominatorTree>();

Usual style is a space before the * not after.

> +      if (Visited.count(BB))
> +        continue;
> +      Visited.insert(BB);

Here you can just do:
   if (!Visited.insert(BB)) continue;

> +      DomTreeNode* Node = DT->getNode(BB);
> +      const std::vector<DomTreeNode*>&  Children = Node->getChildren();
> +      for (unsigned i = 0; i<  Children.size(); ++i) {
> +        // Only visit children that are in the same loop.
> +        BasicBlock* ChildBB = Children[i]->getBlock();
> +        if (!Visited.count(ChildBB)&&  LI->getLoopFor(ChildBB) == L)
> +          VisitStack.push_back(ChildBB);
> +      }

As mentioned above, I don't see the point of using the domtree for this.

> +  // Nothing that SimplifyInstruction() does should invalidate LCSSA form.
> +  assert(L->isLCSSAForm(*DT));

As discussed off list this is not true, as InstructionSimplify may look through
phi nodes.

Ciao, Duncan.