[PATCH] D90328: Eliminates dead store of an exisiting value

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for the update! I think it would be best to adjust the existing tests so that this optimization does not trigger, because they test different things. You can use some of those tests in a new file (something like `MSSA/redundant-stores.ll`), together with the one for PR16520.

In D90328#2428525 <https://reviews.llvm.org/D90328#2428525>, @lebedev.ri wrote:

> Terminology check: the store being eliminated isn't so much dead,
> it's that the stored value is identical to the currently-stored value,
> i.e. the store is effectively a no-op.

Yep, it would be better to refer to redundant or no-op stores.

> Just to check, there is a test for the following:
>
>   int data;
>   data = 0; // keep
>   data = 0; // eliminate
>   data = 1; // keep
>   data = 0; // DON'T eliminate, since last stored value is 1.
>   data = 0; // eliminate
>   data = 1; // keep
>   data = 1; // eliminate
>
> ?

I think in this particular case, the last store kills all other stores regardless of the value, so this transform wouldn't trigger. But we need more unit tests specifically for the transform in the patch.



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:2388
+  /// is already stored in the variable
+  bool eliminateDeadStoresOfExisitingValues() {
+    bool MadeChange = false;
----------------
Instead of talking about dead stores, I think using the term 'redundant' would be better. 


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:2411
+          if (UseInst->isIdenticalTo(DefInst)) {
+            deleteDeadInstruction(UseInst);
+            MadeChange = true;
----------------
Could you add a debug message and increment `      NumRedundantStores` similar to the code around line 2682?


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:2410
+          }
+          Instruction *UseInst = cast<MemoryUseOrDef>(UseDefAccess)->getMemoryInst();
+          if (UseInst == NULL) {
----------------
fhahn wrote:
> You need to check if UseDefAccess is actually a use or def. It could be a MemoryPhi.
> 
> You could use something like `MemoryAccess *UseDefAccess = dyn_cast_or_null<MemoryUseOrDef>(UseDef->getDefiningAccess())`
> 
> Test case
> ```
> define void @main(i32* %a, i32* %b, i1 %c) {
> entry:
>   br label %for.body
> 
> for.body:                                         ; preds = %if.end8, %entry
>   br i1 %c, label %exit, label %loop.latch
> 
> loop.latch:                                          ; preds = %for.body
>   store i32 0 , i32* %a
>   store i32 1, i32* %b
>   br label %for.body
> 
> exit:                                         ; preds = %for.body
>   call void @foo()
>   ret void
> }
> 
> declare void @foo()
> ```
Could you add this case as unit tests?


================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/multiblock-loops.ll:186
 ; CHECK:       for.body:
-; CHECK-NEXT:    store i32 1, i32* [[P]], align 4
 ; CHECK-NEXT:    [[LV:%.*]] = load i32, i32* [[P]], align 4
----------------
I think here it would be better to change the test to make sure the optimization does not trigger, to preserve the original spirit of the test.



================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/multiblock-memoryphis.ll:17
 ; CHECK:       bb3:
-; CHECK-NEXT:    store i32 0, i32* [[P]], align 4
 ; CHECK-NEXT:    ret void
----------------
I think here it would be better to change the test to make sure the optimization does not trigger, to preserve the original spirit of the test.



================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/multiblock-simple.ll:41
 ;
   store i32 0, i32* %P
   br i1 true, label %bb1, label %bb2
----------------
I think here it would be better to change the test to make sure the optimization does not trigger, to preserve the original spirit of the test.


================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/multiblock-simple.ll:102
 ; CHECK:       bb3:
-; CHECK-NEXT:    store i32 0, i32* [[P]], align 4
 ; CHECK-NEXT:    ret void
----------------
I think here it would be better to change the test to make sure the optimization does not trigger, to preserve the original spirit of the test.



================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/multiblock-simple.ll:156
 ; CHECK:       bb1:
-; CHECK-NEXT:    store i32 0, i32* [[P]], align 4
 ; CHECK-NEXT:    br label [[BB3:%.*]]
----------------
I think here it would be better to change the test to make sure the optimization does not trigger, to preserve the original spirit of the test.



================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/pr16520.ll:5
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
----------------
this should not be required for DSE.


================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/pr16520.ll:7
+
+define void @_Z1fbRb(i1 zeroext %b, i8* nocapture %r) {
+; CHECK-LABEL: @_Z1fbRb(
----------------
Could you also add some more variants of the scenarios from the other tests?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90328/new/

https://reviews.llvm.org/D90328