[PATCH] D115829: Allow calls with known writes when trying to remove allocas

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2580
+      continue;
+    if (UsedV != CB.getArgOperand(i) && !CB.onlyReadsMemory(i))
+      // A write to another memory location keeps the call live, and thus we
----------------
nikic wrote:
> The logic here doesn't look right because you're conflating two different cases. If you have a non-UsedV argument that is readonly, then you want to skip that, but currently it will fail because you then go to the code below, which also checks for writeonly. You should be checking nocapture (always) and then for UsedV writeonly and non-UsedV readonly. A test case for a readonly argument seems to be missing as well.
I agree with checking for `nocapture` always first and yes, looks like the "readonly Non-UsedV" case will not be handled. Note that it's just a missed optimization, not an incorrect one. 

Since changing the checks around will address it, it's perhaps worth doing so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115829