[PATCH] D112313: [DSE] Optimize defining access of defs while walking upwards.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 2 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1422
+                     TLI)) {
+        CanOptimize &= !isa<FenceInst>(CurrentDef->getMemoryInst());
         continue;
----------------
nikic wrote:
> Looking at how canSkipDef() is defined, I think this is wrong on two fronts:
> 
> First, canSkipDef() skips any mayThrow() instruction if `!DefVisibleToCaller`. But what if this is a non-nounwind call that clobbers KillingLoc?
> 
> Second, canSkipDef() skips lifetime intrinsics, but I believe those are considered clobbering by MSSA (they effectively write an undef value to the location).
Good point,  it's probably best to disable optimizations for any skipable def to start with.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1477
         LLVM_DEBUG(dbgs() << "  ... not guaranteed loop independent\n");
         WalkerStepLimit -= 1;
+        CanOptimize = false;
----------------
nikic wrote:
> On an unrelated note, why does this explicitly adjust WalkerStepLimit?
I'm not sure, but looking at other continues we should be able to drop it.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:2187
 #endif
-
   if (!Changed)
----------------
nikic wrote:
> Spurious change
Removed it here, but I also need to remove the one below as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112313