[DSE] Remove alloc function check in canSkipDef()

[nikic]

Summary

canSkipDef() currently skips inaccessiblememonly calls, but not if they are allocation functions. This check was added in D103009, but I don't understand why. There are no test failures if it is removed, and I don't really see what relation it has to the implemented transform (as canSkipDef() is not used on the storeIsNoop() path, as far as I can tell).

Either this is dead code, or we're missing a test.

[reames]

FWIW, I'm also quite confused by this code. My best guess was that maybe this was handling the case where we introduced a new memory region (e.g. the return noalias) as opposed to merely writing to an existing region, but that's somewhat a guess. I don't see that distinction made in the DSE code, and I'm not sure how memory-ssa models that.

[yurai007]

Hi, it appears that https://reviews.llvm.org/D103009 emerged from https://reviews.llvm.org/D101440 and canSkipDef change was included by mistake.
It should be safe to get rid of it. Also together with isAllocLikeFn we can remove TLI parameter I think.

[fhahn]

LGTM, thanks!

llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp

702

as @yurai007 suggested, it looks like the TLI arg can be dropped

llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp

if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(I)) {
     default:
       return false;
     }
   }
   return false;
 }
 
 // Check if we can ignore \p D for DSE.
-bool canSkipDef(MemoryDef *D, bool DefVisibleToCaller,
-                const TargetLibraryInfo &TLI) {
+bool canSkipDef(MemoryDef *D, bool DefVisibleToCaller) {
   Instruction *DI = D->getMemoryInst();
   // Calls that only access inaccessible memory cannot read or write any memory
   // locations we consider for elimination.
   if (auto *CB = dyn_cast<CallBase>(DI))
-    if (CB->onlyAccessesInaccessibleMemory()) {
-      if (isAllocLikeFn(DI, &TLI))
-        return false;
+    if (CB->onlyAccessesInaccessibleMemory())
       return true;
-    }
+
   // We can eliminate stores to locations not visible to the caller across
   // throwing instructions.
   if (DI->mayThrow() && !DefVisibleToCaller)
     return true;
 
   // We can remove the dead stores, irrespective of the fence and its ordering
   // (release/acquire/seq_cst). Fences only constraints the ordering of
   // already visible stores, it does not make a store visible to other
for (;; Current = cast<MemoryDef>(Current)->getDefiningAccess()) {
         return Current;
       }
 
       // Below, check if CurrentDef is a valid candidate to be eliminated by
       // KillingDef. If it is not, check the next candidate.
       MemoryDef *CurrentDef = cast<MemoryDef>(Current);
       Instruction *CurrentI = CurrentDef->getMemoryInst();
 
-      if (canSkipDef(CurrentDef, !isInvisibleToCallerBeforeRet(KillingUndObj),
-                     TLI)) {
+      if (canSkipDef(CurrentDef,
+                     !isInvisibleToCallerBeforeRet(KillingUndObj))) {
         CanOptimize = false;
         continue;
       }
 
       // Before we try to remove anything, check for any extra throwing
       // instructions that block us from DSEing
       if (mayThrowBetween(KillingI, CurrentI, KillingUndObj)) {
         LLVM_DEBUG(dbgs() << "  ... skip, may throw!\n");