[PATCH] D71478: [BasicBlockUtils] Add utility to remove redundant dbg.value instrs

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope marked 3 inline comments as done.
bjope added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DCE.cpp:105
+  void getAnalysisUsage(AnalysisUsage &AU) const override {
+    AU.setPreservesCFG();
+  }
----------------
vsk wrote:
> Does this imply that all analyses are preserved? If not, should it?
I'm not sure that it would be correct to use setPreservesAll. The pass is modifying both IR and possibly metadata. Maybe we could list some additional analyses that are known to be preserved, but as long as this pass just is used for lit tests I guess it isn't that important which passes that are preserved.


================
Comment at: llvm/lib/Transforms/Utils/BasicBlockUtils.cpp:427
+  // identical mapping earlier in the basic block.
+  MadeChanges |= removeRedundantDbgInstrsUsingForwardScan(BB);
+
----------------
vsk wrote:
> Could you include a short example that demonstrates why neither the forward nor backward scans alone are sufficient to eliminate all redundancies? Maybe:
> 
> ```
> 1) dbg.value X1, "x", FragmentX1
> <block of instructions, none being "dbg.value ..., "x", ...">
> 2) dbg.value X1, "x", FragmentX1
> 3) dbg.value X2, "x", FragmentX2
> 4) dbg.value X1, "x", FragmentX1
> 5) dbg.value X1, "x", FragmentX1
> ```
> 
> IIUC the backwards pass would delete (4), and the forwards pass would delete (2). Not sure if that's minimal.
Added some comments to highlight the idea behind running two separate scans, and that the order is important.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71478