[PATCH] D29866: [PDSE] Add PDSE.

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

Thanks for this very good piece of work, Bryant.
In general this pass needs more comments (i.e. the phases of the algorithm need to be described, for example `renamePass()`).
Some comments to begin with.



================
Comment at: lib/Transforms/Scalar/PDSE.cpp:195-199
+  SmallVector<RealUse, 4> Uses;
+  // ^ Real occurrences for which this lambda is representative (a def). Each
+  // use will be in the same redundancy class as this lambda (meaning that the
+  // stores they represent are same-sized and must-alias), but can have
+  // different sub-class indexes (memset, memcpy, plain store, etc.).
----------------
We generally put the comment above the field and not below, can you change those?


================
Comment at: lib/Transforms/Scalar/PDSE.cpp:474-478
+    return SI->getValueOperand();
+  } else if (auto *MI = dyn_cast<MemSetInst>(&I)) {
+    return MI->getValue();
+  } else if (auto *MI = dyn_cast<MemTransferInst>(&I)) {
+    return MI->getRawSource();
----------------
you don't need `else` after `return`.


================
Comment at: lib/Transforms/Scalar/PDSE.cpp:548
+  DenseMap<const BasicBlock *, BlockInfo> Blocks;
+  std::forward_list<Instruction *> DeadStores;
+  std::vector<RedClass> Worklist;
----------------
why a forward list?


================
Comment at: lib/Transforms/Scalar/PDSE.cpp:920
+    if (!PDT.getRootNode()) {
+      DEBUG(dbgs() << "FIXME: ran into the PDT bug. nothing we can do.\n");
+      return false;
----------------
Can you please describe the `PDT` bug? I'm not really familiar with it (or I call it with another name :)


Repository:
  rL LLVM

https://reviews.llvm.org/D29866