[PATCH] D19338: New code hoisting pass based on GVN (optimistic approach)

[Daniel Berlin via llvm-commits]

On Tue, May 17, 2016 at 2:23 PM, Aditya Kumar <hiraditya at msn.com> wrote:

> hiraditya added inline comments.
>
> ================
> Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:213
> @@ +212,3 @@
> +
> +  // Return true when all paths from A to the end of the function pass
> through
> +  // either B or C.
> ----------------
> dberlin wrote:
> > Errr, isn't this the definition of A post-dominating B or C?
> >
> >
> > A post-dominates B if all paths from A to end of function pass through B.
> > Same with (A, C).
> >
> > If that's right, i would just use post-dominance here :)
> >
> It is more like B and C combined post dominating A.
>

Yes, sorry, i reversed it.  So why are you not testing that?


>
> ================
> Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:425
> @@ +424,3 @@
> +      BasicBlock *BB = Insn->getParent();
> +      BasicBlock *NewHoistPt = DT->findNearestCommonDominator(HoistPt,
> BB);
> +      WL.insert(BB);
> ----------------
> dberlin wrote:
> > I wonder how expensive this computation ends up being. It never changes
> per-iteration unless something is messing with the CFG out from under you.
> >
> > (This is why GCC uses et-splay trees)
> For each Instruction in the InstructionsToHoist, the nearest common
> dominator (w.r.t. HoistPt) could change.
>
> e.g.,
> A -> B -> C (has I1)
> B-> D (has I2)
> A -> E (has I3).
>
> And if I1, I2 and I3 have the same GVN.
> In this case nearestCommonDominator(C, D) = B, and,
> nearestCommonDominator(B, E) = A.
>

True, but my point was that nearestCommonDominator of any two blocks you
query it for will not change.

You are just saying "what we call it on changes".
But if you call it repeatedly on the same two blocks, you will allways get
the same answer (unless your CFG has changed somehow, and i don't see how
that can happen).

So you could cache NCD into a std::map<std::pair<A,B>, C>.
:)


>
>
> ================
> Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:635-637
> @@ +634,5 @@
> +            continue;
> +          if (!OptForMinSize && !Call->onlyReadsMemory())
> +            CallWithSideEffect = true;
> +          CI.insert(Call, VN);
> +        } else if (!CallWithSideEffect && !isa<GetElementPtrInst>(&I1))
> ----------------
> dberlin wrote:
> > majnemer wrote:
> > > Likewise, you need to make sure that the call has no side effects
> which is different from it not mutating memory.
> > +1
> > Pure and const (in gcc parlance) are pretty much the only thing you can
> safely move.
> >
> >
> >
> Will do that. Thanks.
>
> ================
> Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:665
> @@ +664,3 @@
> +    for (const BasicBlock *BB : depth_first(&F.getEntryBlock()))
> +      DFSNumber.insert(std::make_pair(BB, ++I));
> +
> ----------------
> dberlin wrote:
> > Note that you can get the DFS in/out numbers from the dominator tree if
> that is an acceptable ordering (IE DFS on DT).
> >
> It seems DFS number is not always available in the dominator tree.  DFS
> numbers are updated only if there are too many (> 32) slow queries in
> GenericDomTree.h:468
>
>
> YOu can guarantee it by calling updateDFSNumbers
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160517/db1f5b6e/attachment.html>