[PATCH] D18798: New code hoisting pass based on GVN

[Daniel Berlin via llvm-commits]

I just want to echo Chandler's comments about complexity:
Thank you for taking the time to do the work to not add another N^2 pass to
the compiler, even if it took you a bit more time/effort :)
(I'll take another gander at the patch for code review, but you should
address chandler's comments)

On Thu, Apr 7, 2016 at 1:26 AM, Chandler Carruth <chandlerc at gmail.com>
wrote:

> chandlerc requested changes to this revision.
> chandlerc added a comment.
> This revision now requires changes to proceed.
>
> Some high level comments:
>
> - The code formatting seems bad. Please run clang-format at the least on
> the new code.
> - Thanks for addressing Danny's comments about the algorithmic complexity.
> Keeping this away from O(n^2) algorithms is really important.
> - You didn't address Danny's comment about providing motivating benchmark
> data showing the improvements this provides.
> - We also would likely need to see a reasonable analysis of the effect
> this has on compile time so we understand the tradeoff this is making. My
> suspicion is that it won't be the correct tradeoff for O2 if it is correct
> at any level. (See below.)
>
>
> ================
> Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:212-213
> @@ -211,3 +211,4 @@
>      FPM.add(createScalarReplAggregatesPass());
>    FPM.add(createEarlyCSEPass());
> +  FPM.add(createEarlyGVNPass());
>    FPM.add(createLowerExpectIntrinsicPass());
> ----------------
> It makes very little sense IMO to do early-cse and then early-gvn... The
> only point of early-cse was to be a substantially lighter weight CSE than
> GVN. If you're going to run GVN anyways to do hoisting, just run GVN.
>
> Either way, I strongly suspect this should be conditioned on O3...
>
>
> Repository:
>   rL LLVM
>
> http://reviews.llvm.org/D18798
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160407/da2b3d91/attachment.html>