[PATCH] D47237: [DSE] Calloc-strlen optimizations

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

I can't imagine a situation where this transform would trigger in practice... but maybe I don't write enough C code.   How often does this trigger on some large codebase, like the LLVM testsuite?



================
Comment at: include/llvm/Analysis/MemoryLocation.h:82
+    case Instruction::Call:
+      return get(cast<CallInst>(Inst), Op);
     case Instruction::Load:
----------------
I'd rather not mess with this interface... if the instruction is a call, the caller needs to be more aware of what exactly it's asking for (since it could access any number of different memory locations).


================
Comment at: lib/Analysis/MemoryLocation.cpp:23
+  AAMDNodes AATags;
+  CI->getAAMetadata(AATags);
+
----------------
TBAA metadata doesn't get applied to calls (I'm not sure what the semantics would be), so you probably don't need to call getAAMetadata.


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:581
+                                       Instruction *SecondI, AliasAnalysis *AA,
+                                       const Value *Op = nullptr) {
+  if (FirstI == SecondI)
----------------
Can you just make the MemoryLocation a parameter here?  I don't think your changes to MemoryLocation.h are useful.


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:1085
+  if (N->isNullValue() || Size->isNullValue())
+    return false;
+
----------------
There's no point to checking whether one of the arguments is zero.  We can assume they're non-zero because `strlen(nullptr)` has undefined behavior.


https://reviews.llvm.org/D47237