[PATCH] D19821: [EarlyCSE] Port to use MemorySSA (disabled by default). NFC.

[Daniel Berlin via llvm-commits]

So, if it's not actually slower in practice, would that address your
objection

On Thu, May 5, 2016, 6:31 PM Philip Reames <listmail at philipreames.com>
wrote:

> reames requested changes to this revision.
> reames added a comment.
> This revision now requires changes to proceed.
>
> At a meta level, I'm not convinced that updating EarlyCSE to work with
> MemorySSA is the right approach.  EarlyCSE is focused on being really
> really fast at cleaning up stupidly redundant IR so that the rest of the
> pass pipeline doesn't need to worry about it.  MemorySSA is relatively
> expensive to construct.  Given that, I'm really not sure putting it at the
> very beginning of the pipeline is a good design choice.
>
> Now, having said that, it might make sense to have a dominance order CSE
> pass based on MemorySSA for use later in the pass pipeline.  Currently we
> use EarlyCSE for two distinct purposes, its possible that it might be time
> to split them.
>
> Can you justify why this is the right approach?
>
>
> ================
> Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:511
> @@ +510,3 @@
> +                                       MemorySSAWalker *MSSAWalker) {
> +  if (!CanSkipReleaseFence)
> +    return MA;
> ----------------
> Huh?  This should be handled entirely inside MemorySSA?
>
> ================
> Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:562
> @@ +561,3 @@
> +
> +  MemoryAccess *LaterHeapGen =
> +      getHeapGeneration(LaterInst, CanSkipReleaseFence, MSSA, MSSAWalker);
> ----------------
> Having two sets of variables, one integers, one pointers with similar
> names is *highly* confusing.  I'd suggest pulling out a MemorySSA specific
> impl function and calling it from here to wrap the desired asserts.
>
> ================
> Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:570
> @@ +569,3 @@
> +
> +  // handle case of e.g. load atomic -> store unordered
> +  if (LaterHeapGen == EarlierMA)
> ----------------
> mcrosier wrote:
> > Please capitalize handle and add a period.
> This comment doesn't make sense where placed?
>
> ================
> Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:682
> @@ -578,1 +681,3 @@
> +      // get removed in the code above and MemorySSA updated correctly.
> +      removeMSSA(Inst);
>        Inst->eraseFromParent();
> ----------------
> Code like this strongly hints that MemorySSA should be using ValueHandles.
>
> ================
> Comment at: test/Transforms/EarlyCSE/memoryssa.ll:7
> @@ +6,3 @@
> +;; Simple load value numbering across non-clobbering store.
> +; CHECK-LABEL: @test1(
> +define i32 @test1() {
> ----------------
> If we do go this way, you'll need far far more tests.
>
>
> http://reviews.llvm.org/D19821
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160506/1ba6eeeb/attachment.html>