[PATCH] D11143: [RFC] Cross Block DSE

[Karthik Bhat via llvm-commits]

karthikthecool marked 2 inline comments as done.
karthikthecool added a comment.

Hi Erik,
Thanks for looking into the patch. Please find my comments inline. Will post the updated patch shortly.
Thanks and Regards
Karthik Bhat


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:949
@@ +948,3 @@
+
+  for (auto BBE = SrcBlock->end(); BBI != BBE; ++BBI) {
+    Instruction *I = BBI;
----------------
eeckstein wrote:
> As I understand, this loop is just to get the iterator for I.
> You can get it simply by:
> BasicBlock::iterator BBI(SI);
Updated the code.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:959
@@ +958,3 @@
+    Instruction *I = BI;
+    ModRefInfo Res = AA->getModRefInfo(I, Pointer, PtrSize);
+    if (Res != MRI_NoModRef) {
----------------
eeckstein wrote:
> I think using PtrSize here is wrong. It should be the size of the memory access. This can give you a wrong NoModRef, e.g. if I is a load of a global which has a smaller size than the pointer size.
> 
> Maybe you should just get the MemoryLocation of SI instead of Pointer+PtrSize and use AA->getModRefInfo(I, Location).
Thanks updated the code.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:1011
@@ +1010,3 @@
+        continue;
+      // A path with backedge may not be safe.Conservatively mark
+      // this store unsafe.
----------------
eeckstein wrote:
> Why is a path with a backedge not safe?
Hi Erik,
I came across the following code from MultiSource/Benchmarks/MiBench/consumer-typeset benchmark because of which i conservately added this check. The below .ll snippet is from file z36.c TrieRead function-
  for.cond:                                         ; preds = %for.inc, %if.else.618
    %indvars.iv1644 = phi i64 [ %indvars.iv.next1645, %for.inc ], [ 0, %if.else.618 ]
    %j.0 = phi i32 [ %j.1, %for.inc ], [ 1, %if.else.618 ]
    %prev.0 = phi i32 [ %prev.1, %for.inc ], [ 56, %if.else.618 ]
    %arrayidx626 = getelementptr inbounds [512 x i8], [512 x i8]* %str, i64 0, i64 %indvars.iv1644
    %42 = load i8, i8* %arrayidx626, align 1, !tbaa !18
    switch i8 %42, label %if.else.636 [
      i8 0, label %for.end
      i8 45, label %for.inc
    ]

  if.else.636:                                      ; preds = %for.cond
    %idxprom639 = sext i32 %j.0 to i64
    %arrayidx640 = getelementptr inbounds [512 x i8], [512 x i8]* %key, i64 0, i64 %idxprom639
    store i8 %42, i8* %arrayidx640, align 1, !tbaa !18
    %conv641 = trunc i32 %prev.0 to i8
    %inc642 = add nsw i32 %j.0, 1
    %arrayidx644 = getelementptr inbounds [512 x i8], [512 x i8]* %value, i64 0, i64 %idxprom639
    store i8 %conv641, i8* %arrayidx644, align 1, !tbaa !18
    br label %for.inc

  for.inc:                                          ; preds = %for.cond, %if.else.636
    %j.1 = phi i32 [ %inc642, %if.else.636 ], [ %j.0, %for.cond ]
    %prev.1 = phi i32 [ 56, %if.else.636 ], [ 57, %for.cond ]
    %indvars.iv.next1645 = add nuw nsw i64 %indvars.iv1644, 1
    br label %for.cond

  for.end:                                          ; preds = %for.cond
    %prev.0.lcssa = phi i32 [ %prev.0, %for.cond ]
    %j.0.lcssa = phi i32 [ %j.0, %for.cond ]
    %idxprom647 = sext i32 %j.0.lcssa to i64
    %arrayidx648 = getelementptr inbounds [512 x i8], [512 x i8]* %key, i64 0, i64 %idxprom647
    store i8 46, i8* %arrayidx648, align 1, !tbaa !18
    %conv649 = trunc i32 %prev.0.lcssa to i8
    %inc650 = add nsw i32 %j.0.lcssa, 1
    %arrayidx652 = getelementptr inbounds [512 x i8], [512 x i8]* %value, i64 0, i64 %idxprom647
    store i8 %conv649, i8* %arrayidx652, align 1, !tbaa !18
    %idxprom653 = sext i32 %inc650 to i64
    %arrayidx654 = getelementptr inbounds [512 x i8], [512 x i8]* %key, i64 0, i64 %idxprom653
    store i8 0, i8* %arrayidx654, align 1, !tbaa !18
    %arrayidx657 = getelementptr inbounds [512 x i8], [512 x i8]* %value, i64 0, i64 %idxprom653
    store i8 56, i8* %arrayidx657, align 1, !tbaa !18
    %add658 = add nsw i32 %j.0.lcssa, 2
    %idxprom659 = sext i32 %add658 to i64
    %arrayidx660 = getelementptr inbounds [512 x i8], [512 x i8]* %value, i64 0, i64 %idxprom659
    store i8 0, i8* %arrayidx660, align 1, !tbaa !18
    %call666 = call fastcc i32 @TrieInsert(i8* %5, i8* %6, %struct.trie_rec* %11, i8* %arraydecay, i32 %inc)
    %tobool667 = icmp eq i32 %call666, 0
    br i1 %tobool667, label %if.then.668, label %while.cond.224.backedge

Here block  for.end post dominates  if.else.636 and for.inc and alias analysis returns MustAlias for the pair-
   store i8 %42, i8* %arrayidx640, align 1, !tbaa !18
   and
   store i8 46, i8* %arrayidx648, align 1, !tbaa !18
and
   store i8 %conv649, i8* %arrayidx652, align 1, !tbaa !18
   and
   store i8 %conv641, i8* %arrayidx644, align 1, !tbaa !18
When we do a DFS from block  if.else.636 to see if it is safe to delete the store we take a backedge and reach  for.end were we find a MustAlias Store and hence delete the store in the block   if.else.636. But this is incorrect as the store in  if.else.636 is inside a loop and stores to various memory location based on the iterator which is updated in for.cond but the store in for.end:  stores to a single location after the loop is executed. 
I came across this issue during testing hence conservately added a check not to take backedges during DFS. 
I'm not sure if there is any other way to fix this. Please let me know if you have any inputs on this. It would be of great help.
Thanks


http://reviews.llvm.org/D11143