[PATCH] D11143: [RFC] Cross Block DSE

[Daniel Berlin via llvm-commits]

I think we should just fix the PDT in a separate patch, and remove this check.

The documentation for getRootNode says:
"  /// getRootNode - This returns the entry node for the CFG of the
function.  If
   /// this tree represents the post-dominance relations for a
function, however,
   /// this root may be a node with the block == NULL.  This is the case when
   /// there are multiple exit nodes from a particular function.  Consumers of
   /// post-dominance information must be capable of dealing with this
   /// possibility."

Note: This may be a node with *block* == NULL, not that the root node
may be null.

The problem here is in PDT building.

GenericDomTreeConstruction does this:
 if (TraitsTy::child_begin(I) == TraitsTy::child_end(I))
           addRoot(I);


This is wrong, obviously, since in the case of an infinite loop like
you've shown, it will have a self-loop, but it is a root because there
is no exit.

This whole mechanism of trying to detect the roots is broken, in fact.
It's essentially trying to determine reachability without a DFS, which
is not possible in general.

The *only* correct way (i'm aware of) to determine which things need
to be children of artificial exit/a root is to DFS from all clear exit
blocks, and then, for anything still unvisited, DFS from each of them,
marking them as children of the artificial exit
(you could also reparent anything you reach  that was marked as a
child of artificial exit , if we want to prefer anything over the
artificial exit as a parent. But this should not be necessary for
correct answers).


Please file a bug with your testcase and cc me, and i'l update the bug
with details.

(I'll see if i can find time to prepare a patch)



On Mon, Aug 10, 2015 at 3:37 AM, Karthik Bhat <kv.bhat at samsung.com> wrote:
> karthikthecool marked 2 inline comments as done.
> karthikthecool added a comment.
>
> Hi Daniel,Hal,
> Thanks for the review. Please find my comments inline. I will try to upload the updated patch shortly.
> Thanks and Regards
> Karthik
>
>
> ================
> Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:101
> @@ +100,3 @@
> +      PDT = &getAnalysis<PostDominatorTree>();
> +      if (PDT->getRootNode()) {
> +        int Count = PDT->getRootNode()->getDFSNumOut();
> ----------------
> dberlin wrote:
>> Do you have a testcase for where PDT->getRootNode() is null?
>>
>> because that seems broken :)
>>
> Hi Daniel,
> I found in the below case we were getting getRootNode as NULL so added the above check.
>   void foo() {
>     while(1);
>   }
>
> ================
> Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:108
> @@ +107,3 @@
> +        // If we have more than 1 block try to populate candidate store.
> +        if (Count > 1)
> +          populateCandidateStores(F);
> ----------------
> dberlin wrote:
>> So, if we only have 1 block, why are you doing the rest of the work here, instead of returning false and exiting?
>> :)
>>
> Updated code to executed FindFunctionBackedges and BackEdgesMap population only if Count > 1. I think we cannot return false here because we still need to do single block dse.
>
>
> http://reviews.llvm.org/D11143
>
>
>