[llvm] r229059 - [unroll] Change the other worklist in the unroll analyzer to be a set

[Hal Finkel]

----- Original Message -----
> From: "Chandler Carruth" <chandlerc at gmail.com>
> To: llvm-commits at cs.uiuc.edu
> Sent: Thursday, February 12, 2015 10:27:50 PM
> Subject: [llvm] r229059 - [unroll] Change the other worklist in the unroll	analyzer to be a set
> 
> Author: chandlerc
> Date: Thu Feb 12 22:27:50 2015
> New Revision: 229059
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=229059&view=rev
> Log:
> [unroll] Change the other worklist in the unroll analyzer to be a set
> vector.
> 
> In addition to dramatically reducing the work required for contrived
> example loops, this also has to correct some serious latent bugs in
> the
> cost computation. Previously, we might add an instruction onto the
> worklist once for every load which it used and was simplified. Then
> we
> would visit it many times and accumulate "savings" each time.

I thought the check against CountedInstructions prevented this?

    if (SimpleV && CountedInstructions.insert(&I).second)
      NumberOfOptimizedInstructions += TTI.getUserCost(&I);


 -Hal

> 
> I mean, fortunately this couldn't matter for things like calls with
> 100s
> of operands, but even for binary operators this code seems like it
> must
> be double counting the savings.
> 
> I just noticed this by inspection and due to the runtime problems it
> can
> introduce, I don't have any test cases for cases where the cost
> produced
> by this routine is unacceptable.
> 
> Modified:
>     llvm/trunk/lib/Transforms/Scalar/LoopUnrollPass.cpp
> 
> Modified: llvm/trunk/lib/Transforms/Scalar/LoopUnrollPass.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/LoopUnrollPass.cpp?rev=229059&r1=229058&r2=229059&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Scalar/LoopUnrollPass.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/LoopUnrollPass.cpp Thu Feb 12
> 22:27:50 2015
> @@ -456,7 +456,7 @@ public:
>    // values for the given Iteration.
>    // Fill in SimplifiedValues map for future use in DCE-estimation.
>    unsigned estimateNumberOfSimplifiedInstructions(unsigned
>    Iteration) {
> -    SmallVector<Instruction *, 8> Worklist;
> +    SmallSetVector<Instruction *, 8> Worklist;
>      SimplifiedValues.clear();
>      CountedInstructions.clear();
>      NumberOfOptimizedInstructions = 0;
> @@ -474,7 +474,7 @@ public:
>            continue;
>          if (!L->contains(UI))
>            continue;
> -        Worklist.push_back(UI);
> +        Worklist.insert(UI);
>        }
>      }
>  
> @@ -491,7 +491,7 @@ public:
>            continue;
>          if (!L->contains(UI))
>            continue;
> -        Worklist.push_back(UI);
> +        Worklist.insert(UI);
>        }
>      }
>      return NumberOfOptimizedInstructions;
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory