[PATCH] D48383: [Dominators] Add the DomTreeUpdater class

[Brian Rzycki via Phabricator via llvm-commits]

brzycki added a comment.

@kuhar and @NutshellySima here are a few more small comments.



================
Comment at: include/llvm/IR/DomTreeUpdater.h:60
+  ///
+  /// \returns true if all holding data structures are up to date.
+  /// \returns false otherwise.
----------------
kuhar wrote:
> Can you clarify what happens in eager vs. lazy scenario?
+1. I'd assume for `Eager` the routine always returns false since queuing isn't allowed. It'd be good to state that explicitly.


================
Comment at: include/llvm/IR/DomTreeUpdater.h:58
+
+  /// Returns false if there isn't ant pending update for the holding trees
+  /// regardless of whether there is BasicBlock awaiting deletion.
----------------
kuhar wrote:
> I'm not sure that `holding trees` is grammatically correct in this place and all the other ones.
> Maybe `held trees` or just `available trees`.
> Perhaps @brzycki could help here.
The phrase `holding trees` doesn't make sense in this context. I didn't mention it before because I'm not an expert on Dominators and wasn't sure if this was a technical phrase. :)

If you need to keep the phrase I'd recommend `held trees` but even that sounds a bit odd. Something along the lines of `valid tree instances` or `trees contained inside`. I tend to think of the DTU object instantiated with one or two tree objects.

In this particular case the english is odd when describing the false case. I'd prefer if the comment said:
```
/// Returns true if either of DT or PDT is valid and the tree has at
/// least one update pending. If DT or PDT is nullptr it is treated
/// as having no pending updates.
```

Something like that I think is a bit easier to understand.


================
Comment at: lib/IR/DomTreeUpdater.cpp:248
+  while (!DelBB->empty()) {
+    Instruction &I = DelBB->back();
+    // Replace used instructions with an arbitrary value (undef).
----------------
kuhar wrote:
> Can a BB have 0 instructions?
Not for long.  The only place I've seen BBs with 0 instructions is just before actual deletion. Too many routines expect, at the very least, a valid TI and will cause `assert()` casting `nullptr` to `I` or dereferencing a `nullptr` instruction instance. As much as anything's a rule in the middle end, I've found that to be pretty consistent.


https://reviews.llvm.org/D48383