[PATCH] D29865: [PDSE] Add a no-op pass.

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added a comment.

Just some minor comments/questions and some nitpicking.
Thanks for working on this!
Filipe



================
Comment at: include/llvm/Transforms/Scalar/PDSE.h:1
+//===- PDSE.h - PartialDead Store Elimination -----------------------------===//
+//
----------------
Missing space.


================
Comment at: lib/Transforms/Scalar/PDSE.cpp:43
+//   redundancy graph of escaping stores;
+// - TODO: Figure out partial overwrite tracking.
+//
----------------
>From this TODO, it seems for the foreseeable future (first iterations and commits of this pass), it's not going to cover partial overwrites like D30703 is adding.
Please let me know if my understanding is correct, and if you have plans for adding partial overwrite tracking "soon" (as in: there's already some work underway, or a plan on how to do it), or if it's more of a "after pdse gets in, we might start working on that".

This will help figure out if D30703 should wait for PDSE, or be committed with maybe a TODO mentioning PDSE and that it might not be needed in the future.


================
Comment at: lib/Transforms/Scalar/PDSE.cpp:64
+static cl::opt<bool>
+    PrintFRG("print-frg", cl::init(false), cl::Hidden,
+             cl::desc("Print the factored redundancy graph of stores."));
----------------
Maybe make the option be `-print-pdse-frg` (`print` is more important) or `-pdse-print-frg` (`pdse` is more important), so it's easier to tell where the option will be active. And/or add a PDSE reference to the description.


================
Comment at: lib/Transforms/Scalar/PDSE.cpp:75
+    DEBUG(dbgs() << "Dummy PDSE pass.\n");
+  }
+  return false;
----------------
No brackets after return, please.
Maybe print this last one above the other one, and just have one return false, even.


Repository:
  rL LLVM

https://reviews.llvm.org/D29865