[PATCH] D29866: [PDSE] Add PDSE.

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added a comment.

Will need to read up a bit more before I can usefully comment on the algorithm, but I've added a few minor comments.

Please document the tests a bit better. Some of them have a good explanation of what they're testing, others don't. For example, in the first two, add a high-level explanation of what they're testing. It makes it much easier to figure out what happened when someone else changes some code which changes the output on those tests.



================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:78
+static cl::opt<bool> RunPDSE("enable-pdse", cl::init(false), cl::Hidden,
+                             cl::desc("Run with PDSE instead of DSE."));
+
----------------
davide wrote:
> mehdi_amini wrote:
> > Is "PDSE" supposed to be a well known acronym? I feel that the description could be more friendly with `--help` readers.
> > 
> > It also sounds strange to me that "Partial DSE" is "stronger" than "DSE" alone.
> It doesn't to me, at all. See, for example, PRE (partial redundancy elimination) which is stronger than full redundancy elimination (once we agree on the meaning of stronger).
I think it helps if you look at it as (PDS)E, and not as P(DSE) :-)


================
Comment at: lib/Transforms/Scalar/PDSE.cpp:101
+
+  const RealOcc *isReal() const {
+    return Type == OccTy::Real ? reinterpret_cast<const RealOcc *>(this)
----------------
`isReal`/`isLambda` sound like they return booleans. Code like `X->isReal()->isLambda()` looks weird.
In my opinion, `asReal`/`asLambda` (or similar) would be easier to read (exact same behavior: If they're the type that we're asking for, return the pointer. Otherwise return `nullptr`).


================
Comment at: lib/Transforms/Scalar/PDSE.cpp:157
+
+bool hasMultiplePreds(const BasicBlock &BB, bool AlreadyOnePred = true) {
+  auto B = pred_begin(&BB);
----------------
This is always called without the second parameter.
It seems code would be simpler if you just took a pointer as an argument.


================
Comment at: lib/Transforms/Scalar/PDSE.cpp:171
+    LambdaOcc *getLambda() const {
+      return Inner->isReal() ? Inner->isReal()->isLambda() : Inner->isLambda();
+    }
----------------
Why `isReal()->isLambda()`? Won't the `isLambda()` always return `nullptr`? (`Type` can't be both `OccTy::Real` and `OccTy::Lambda`)


================
Comment at: lib/Transforms/Scalar/PDSE.cpp:282
+  MemoryLocation Loc;
+  // ^ The memory location that each RealOcc mods and must-alias.
+  SmallVector<RedIdx, 8> Overwrites;
----------------
We usually comment above the variable.


================
Comment at: lib/Transforms/Scalar/PDSE.cpp:283
+  // ^ The memory location that each RealOcc mods and must-alias.
+  SmallVector<RedIdx, 8> Overwrites;
+  // ^ Indices of redundancy classes that this class can DSE.
----------------
Did you do any measurements/reasoning for these sizes, btw?
(Just asking. No need to worry too much for now)


================
Comment at: lib/Transforms/Scalar/PDSE.cpp:504
+    return make_pair(MemoryLocation::get(SI), RealOcc(NextID++, I));
+  } else if (auto *MI = dyn_cast<MemSetInst>(&I)) {
+    return make_pair(MemoryLocation::getForDest(MI), RealOcc(NextID++, I));
----------------
Nit: We can do without the `else` in these.


================
Comment at: lib/Transforms/Scalar/PDSE.cpp:921
+      DEBUG(dbgs() << "FIXME: ran into the PDT bug. nothing we can do.\n");
+      return false;
+    }
----------------
Should we assert?


================
Comment at: lib/Transforms/Scalar/PDSE.cpp:1003
+
+// vim: set shiftwidth=2 tabstop=2:
----------------
Please remove editor-related markup.


================
Comment at: test/Transforms/DeadStoreElimination/pdse.ll:233
+
+; http://i.imgur.com/abuFdZ2.png
+define void @multiple_pre(i8* %a, i8 %b, i1 %c, i1 %d) {
----------------
I'd rather have an ASCII diagram instead of relying on an external image service (also hard to look at when reading tests).


Repository:
  rL LLVM

https://reviews.llvm.org/D29866