[PATCH] D135574: [MemCpyOpt] Fix up debug loc for simplified memset in processMemSetMemCpyDependence

Fri May 12 08:18:24 PDT 2023

jmorse added a reviewer: Orlando.
jmorse added a comment.

Makes sense to me, the new comments appear to reflect what the code is doing better (insert the memset before the memcpy). The source locations look good too (added one nit comment). Added Orlando as a reviewer as peeling apart memcpys is his favourite thing.

I've a small concern over whether the new memset is going to re-order source locations -- as defined in https://llvm.org/docs/HowToUpdateDebugInfo.html we try to avoid source locations appearing outside of their original block or out of the original program order. It looks like the only caller to `processMemSetMemCpyDependence` enforces that the memset/memcpy are in the same block which means this patch is fine, but this is liable to become untrue in a future refactor. Would you be able to put a `MemSet->getParent() == MemCpy->getParent()` assertion next to the call to `SetCurrentDebugLocation` plus a comment about avoiding the source location appearing out of order? It feels a bit pointless, but IMO it'll guard against future refactors making a mistake.

================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1291
+  // The new memset is inserted before the memcpy, and it is known that the
+  // memcpy:s defining access is the memset about to be removed.
   auto *LastDef =
----------------

================
Comment at: llvm/test/Transforms/MemCpyOpt/memset-memcpy-dbgloc.ll:25-28
+; Validate that the memset is mapped to DILocation for the original memset.
+; CHECK: [[DBG11]] = !DILocation(line: 1, column: 1, scope: !5)
+; CHECK: [[DBG12]] = !DILocation(line: 2, column: 1, scope: !5)
+; CHECK: [[DBG13]] = !DILocation(line: 3, column: 1, scope: !5)
----------------
I'd suggest dropping the scope metadata node as it's liable to be re-numbered by output changes in the future, and in this context I don't think it's testing anything meaningful.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135574