[llvm] r342055 - [GVNHoist] computeInsertionPoints() miscalculates IDF

[Philip Reames via llvm-commits]

I've noticed another round of work recently on fixing issues in 
GVNHoist.  First, let me start by saying, thank you for being responsive 
to issues and disabling quickly while investigation is in progress.  
That makes this much less disruptive for the rest of the community.

However, I would like to suggest a change in approach from the current 
"enable and see what breaks" approach.

We have multiple community fuzzing efforts currently active. Many of the 
issues being found seem like things that fuzzers would be good at 
uncovering.  Can I request that you make a proactive effort to leverage 
testing infrastructure such a fuzzers before re-enabling GVNHoist by 
default?

Specifically, we have the following tooling:

  * csmith is available and I believe regularly run.
  * We have the llvm-opt-fuzzer infrastructure which is run continuously
    on OSSFuzz.  Adapting this to exercise GVNHoist should be
    straight-forward.  (see https://llvm.org/docs/FuzzingLLVM.html)
  * We (Azul) have a Java fuzzer which has found several subtle issues
    in GVN itself.  We can run a few cycles for you at some point, but
    since exposing the failing test cases is very manual for us, I'd ask
    you explore the other options first.

An alternate approach would be to identify one or more large projects - 
say Chromium, Clang itself -, build with GVN hoist enabled, and debug 
any failures which result.  Lest this is not misread, let me be quick to 
say that I haven't followed this effort closely.  It's entirely possible 
this has already been done and this is simply not visible to me as a 
casual observer.  If so, it might be helpful to spell out what has 
already been done and link to it from future re-enable commits.

Philip


On 09/12/2018 07:28 AM, Alexandros Lamprineas via llvm-commits wrote:
> Author: alelab01
> Date: Wed Sep 12 07:28:23 2018
> New Revision: 342055
>
> URL: http://llvm.org/viewvc/llvm-project?rev=342055&view=rev
> Log:
> [GVNHoist] computeInsertionPoints() miscalculates IDF
>
> Fix for https://bugs.llvm.org/show_bug.cgi?id=38912.
>
> In GVNHoist::computeInsertionPoints() we iterate over the Value
> Numbers and calculate the Iterated Dominance Frontiers without
> clearing the IDFBlocks vector first. IDFBlocks ends up accumulating
> an insane number of basic blocks, which bloats the compilation time
> of SemaChecking.cpp with ubsan enabled.
>
> Differential Revision: https://reviews.llvm.org/D51980
>
> Modified:
>      llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp
>
> Modified: llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp?rev=342055&r1=342054&r2=342055&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp Wed Sep 12 07:28:23 2018
> @@ -155,7 +155,6 @@ struct CHIArg {
>   
>   using CHIIt = SmallVectorImpl<CHIArg>::iterator;
>   using CHIArgs = iterator_range<CHIIt>;
> -using CHICache = DenseMap<BasicBlock *, SmallPtrSet<Instruction *, 4>>;
>   using OutValuesType = DenseMap<BasicBlock *, SmallVector<CHIArg, 2>>;
>   using InValuesType =
>       DenseMap<BasicBlock *, SmallVector<std::pair<VNType, Instruction *>, 2>>;
> @@ -767,7 +766,6 @@ private:
>       ReverseIDFCalculator IDFs(*PDT);
>       OutValuesType OutValue;
>       InValuesType InValue;
> -    CHICache CachedCHIs;
>       for (const auto &R : Ranks) {
>         const SmallVecInsn &V = Map.lookup(R);
>         if (V.size() < 2)
> @@ -786,6 +784,7 @@ private:
>         // which currently have dead terminators that are control
>         // dependence sources of a block which is in NewLiveBlocks.
>         IDFs.setDefiningBlocks(VNBlocks);
> +      IDFBlocks.clear();
>         IDFs.calculate(IDFBlocks);
>   
>         // Make a map of BB vs instructions to be hoisted.
> @@ -798,8 +797,7 @@ private:
>           for (unsigned i = 0; i < V.size(); ++i) {
>             CHIArg C = {VN, nullptr, nullptr};
>              // Ignore spurious PDFs.
> -          if (DT->properlyDominates(IDFB, V[i]->getParent()) &&
> -              CachedCHIs[IDFB].insert(V[i]).second) {
> +          if (DT->properlyDominates(IDFB, V[i]->getParent())) {
>               OutValue[IDFB].push_back(C);
>               LLVM_DEBUG(dbgs() << "\nInsertion a CHI for BB: " << IDFB->getName()
>                                 << ", for Insn: " << *V[i]);
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180912/c405d08c/attachment.html>