[PATCH] D48383: [Dominators] Add the DomTreeUpdater class
Jakub Kuderski via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 20 22:14:08 PDT 2018
kuhar requested changes to this revision.
kuhar added a comment.
This revision now requires changes to proceed.
Thanks for the hard work!
I left you mostly stylistic comments. Please take a look at other files in this directory and the surrounding ones and see how people write comments and lay out code. This can vary a lot from a file to file, but it seems to me that the style of this patch is somewhat different to existing ones in Analysis and Transforms.
Please let me know if I'm missing some newest coding style and documentation recommendations, I might be not entirely up-to-date with it.
This patch is pretty big and it will probably take a couple of iterations before we can get it landed.
================
Comment at: include/llvm/IR/DomTreeUpdater.h:28
+ enum class UpdateStrategy : unsigned char { Eager = 0, Lazy = 1 };
+ // Constructor
+ explicit DomTreeUpdater(UpdateStrategy Ups_) : Ups(Ups_) {}
----------------
If you want to visually split the class into a few fragments, maybe it would be better to use some character (like `=====` or `-----`) to make it more apparent? You should be able to grep for it to see what other people use.
================
Comment at: include/llvm/IR/DomTreeUpdater.h:30
+ explicit DomTreeUpdater(UpdateStrategy Ups_) : Ups(Ups_) {}
+ explicit DomTreeUpdater(DominatorTree &DT_, UpdateStrategy Ups_)
+ : DT(&DT_), Ups(Ups_) {}
----------------
What sort of code patterns are you trying to prevent here? I thought it only makes sense to use explicit with constructors taking 1 argument.
================
Comment at: include/llvm/IR/DomTreeUpdater.h:37
+ : DT(&DT_), PDT(&PDT_), Ups(Ups_) {}
+ // Query
+ /// Get UpdateStrategy to enable further low level optimizations
----------------
I think it would be more readable if there was one empty line after a member declaration and a comment below.
(This applies to the rest of the class as well.)
================
Comment at: include/llvm/IR/DomTreeUpdater.h:38
+ // Query
+ /// Get UpdateStrategy to enable further low level optimizations
+ /// in codes.
----------------
This seems to imply that calling this function makes your code run faster :P.
It think it would be enough to explain that you can have eager and lazy updates and what each implies and when to use it.
================
Comment at: include/llvm/IR/DomTreeUpdater.h:42
+ /// Check whether the updater class holds a dominator tree.
+ /// \returns true if it holds a dominator tree.
+ bool hasDomTree() const { return DT != nullptr; }
----------------
This function is trivially self-explanatory, I'm not sure if we want to have a full documentation, including returned values and arguments, for simple getters like this
================
Comment at: include/llvm/IR/DomTreeUpdater.h:46
+ /// \returns true if it holds a post dominator tree.
+ bool hasPostDomTree() const { return PDT != nullptr; }
+ /// Check whether there is Basic Block pending to be deleted.
----------------
Same as above. It think it would be more valuable to explain thinks you cannot guess just by eyeballing function signatures.
================
Comment at: include/llvm/IR/DomTreeUpdater.h:50
+ /// \return true if there is Basic Block pending to be deleted.
+ bool hasPendDeletedBB() const;
+ /// Check whether a specific BasicBlock is waiting to be deleted.
----------------
I think it would be better to stick to the same naming scheme across the file:
here it's "Pend", below is "Pending".
================
Comment at: include/llvm/IR/DomTreeUpdater.h:57
+ /// \returns true when the specific BasicBlock is waiting to be deleted.
+ bool hasPendingDeletedBB(BasicBlock *DelBB) const;
+ /// Check whether there are any pending updates.
----------------
Maybe: isBBPendingDeletion or merge it with the one above?
Not sure about this one, but it's weird to have 2 functions with almost exactly the same name.
================
Comment at: include/llvm/IR/DomTreeUpdater.h:60
+ ///
+ /// \returns true if all holding data structures are up to date.
+ /// \returns false otherwise.
----------------
Can you clarify what happens in eager vs. lazy scenario?
================
Comment at: include/llvm/IR/DomTreeUpdater.h:100
+ /// The callback will be called after DelBB -> removeFromParent()
+ /// , removal of DelBB from trees and before delete
+ /// DelBB.
----------------
nit: dangling `,`
================
Comment at: include/llvm/IR/DomTreeUpdater.h:108
+ /// Recalculate the dominator tree.
+ /// In this scenario, it is supposed the post dominator
+ /// tree should not be recalculated.
----------------
What scenario?
================
Comment at: include/llvm/IR/DomTreeUpdater.h:110
+ /// tree should not be recalculated.
+ /// This function should be called when the class holds a DomTree.
+ void recalculateDomTree(Function &F);
----------------
Should or must?
================
Comment at: include/llvm/IR/DomTreeUpdater.h:110
+ /// tree should not be recalculated.
+ /// This function should be called when the class holds a DomTree.
+ void recalculateDomTree(Function &F);
----------------
kuhar wrote:
> Should or must?
How about something like this: `// Recalculates held DomTree. Caller must ensure there is a DomTree to update.`
================
Comment at: include/llvm/IR/DomTreeUpdater.h:116
+ /// This function should be called when the class holds a PostDomTree.
+ void recalculatePostDomTree(Function &F);
+ /// Recalculate Both of the trees.
----------------
Similar to above.
One more thing: why do we expose these function? Is it possible that someone wants to recalculate just one tree and leave the other one intact? If this is the case, can you explain when it would make sense?
================
Comment at: include/llvm/IR/DomTreeUpdater.h:119
+ /// This function should be called when the class holds both of the trees.
+ void recalculateAll(Function &F);
+ // flush
----------------
Why doesn't it recalculate whatever trees you have?
================
Comment at: include/llvm/IR/DomTreeUpdater.h:158
+ PostDominatorTree *PDT = nullptr;
+ const UpdateStrategy Ups;
+ SmallPtrSet<BasicBlock *, 8> DeletedBBs;
----------------
I think S should be capitalized. Or maybe you could call it `Strategy`? Not sure here.
================
Comment at: include/llvm/IR/DomTreeUpdater.h:170
+ /// \returns false when nothing deleted.
+ /// \returns true otherwise.
+ bool forceFlushDeletedBB();
----------------
maybe: `\returns true is at least one BB was deleted`?
================
Comment at: include/llvm/IR/DomTreeUpdater.h:179
+ BasicBlock *To);
+ /// Apply all pending DomTree updates.
+ void applyDomTreeUpdates();
----------------
I don't think this comments gives any extra information on top of the function signature. Same for the two functions below.
================
Comment at: include/llvm/IR/DomTreeUpdater.h:183
+ void applyPostDomTreeUpdates();
+ /// Flush unnecessary deleted BasicBlocks.
+ void tryFLushDeletedBB();
----------------
Can you explain what you mean by unnecessary?
================
Comment at: include/llvm/IR/DomTreeUpdater.h:190
+ void eraseDelBBNode(BasicBlock *DelBB);
+ /// Determine whether the update is valid or is self-dominance
+ /// in a single step of update or needs to discard in the batch update.
----------------
I like this comment! It's fairly brief yet explain what you consider to be valid and why you need this function.
================
Comment at: lib/IR/DomTreeUpdater.cpp:25
+ const DominatorTree::UpdateType Update) const {
+ const auto From = Update.getFrom();
+ const auto To = Update.getTo();
----------------
When you are using auto with pointers, `auto*` is preferred.
================
Comment at: lib/IR/DomTreeUpdater.cpp:29
+ if (From == To)
+ return false; // Won't affect DomTree and PostDomTree; discard update.
+ // Discard updates by inspecting the current state of successors of From.
----------------
This looks a bit weird with the comment below with no empty space. I would move it above the if.
================
Comment at: lib/IR/DomTreeUpdater.cpp:38
+ if (Kind == DominatorTree::Insert && !HasEdge)
+ return false; // Invalid Insert: edge does not exist in IR.
+ if (Kind == DominatorTree::Delete && HasEdge)
----------------
Invalid sounds like this is some error, do you mean that the updates is a NOP?
================
Comment at: lib/IR/DomTreeUpdater.cpp:76
+ return;
+ if (hasPendingDomTreeUpdates()) {
+ DT->applyUpdates(ArrayRef<DominatorTree::UpdateType>(
----------------
I would put an extra line above.
================
Comment at: lib/IR/DomTreeUpdater.cpp:92
+ return;
+ if (hasPendingPostDomTreeUpdates()) {
+ PDT->applyUpdates(ArrayRef<DominatorTree::UpdateType>(
----------------
Same as above.
================
Comment at: lib/IR/DomTreeUpdater.cpp:120
+void DomTreeUpdater::recalculateAll(Function &F) {
+ assert(DT != nullptr && PDT != nullptr &&
+ "recalculateAll must be called with DomTree and PostDomTree.");
----------------
`assert(DT && PDT)`
================
Comment at: lib/IR/DomTreeUpdater.cpp:127
+ }
+ // Prevent forceFlushDeletedBB() from erasing DomTree and PostDomTree nodes.
+ IsRecalculatingDomTree = IsRecalculatingPostDomTree = true;
----------------
I'd put an empty line here
================
Comment at: lib/IR/DomTreeUpdater.cpp:136
+ PDT->recalculate(F);
+ // Resume updating both two trees after recalculating.
+ IsRecalculatingDomTree = IsRecalculatingPostDomTree = false;
----------------
Also here
================
Comment at: lib/IR/DomTreeUpdater.cpp:142
+}
+// This function should be used when the graph is changed significantly
+// and DT must be recalculated, previous graph changes should be made.
----------------
Empty line as well
================
Comment at: lib/IR/DomTreeUpdater.cpp:142
+}
+// This function should be used when the graph is changed significantly
+// and DT must be recalculated, previous graph changes should be made.
----------------
kuhar wrote:
> Empty line as well
You don't need to repeat the comments, this belongs to the declaration in the header.
================
Comment at: lib/IR/DomTreeUpdater.cpp:145
+void DomTreeUpdater::recalculateDomTree(Function &F) {
+ assert(DT != nullptr && "recalculateDomTree must be called with a DomTree.");
+ if (Ups == UpdateStrategy::Eager) {
----------------
`assert(DT && ...)`
================
Comment at: lib/IR/DomTreeUpdater.cpp:166
+ }
+}
+// This function should be used when the graph is changed significantly
----------------
Empty line please
================
Comment at: lib/IR/DomTreeUpdater.cpp:168
+// This function should be used when the graph is changed significantly
+// and PDT must be recalculated, previous graph changes should be made.
+void DomTreeUpdater::recalculatePostDomTree(Function &F) {
----------------
This should be in the header.
================
Comment at: lib/IR/DomTreeUpdater.cpp:176
+ }
+ // Prevent forceFlushDeletedBB() from erasing PostDomTree nodes.
+ IsRecalculatingPostDomTree = true;
----------------
Empty line please
================
Comment at: lib/IR/DomTreeUpdater.cpp:199
+bool DomTreeUpdater::hasPendingDomTreeUpdates() const {
+ if (DT == nullptr)
+ return false;
----------------
`if (!DT)`
================
Comment at: lib/IR/DomTreeUpdater.cpp:205
+bool DomTreeUpdater::hasPendingPostDomTreeUpdates() const {
+ if (PDT == nullptr)
+ return false;
----------------
`if (!PDT)`
================
Comment at: lib/IR/DomTreeUpdater.cpp:247
+ if (DT && !IsRecalculatingDomTree)
+ if (DT->getNode(DelBB) != nullptr)
+ DT->eraseNode(DelBB);
----------------
`if (DT->getNode(DelBB))`
================
Comment at: lib/IR/DomTreeUpdater.cpp:250
+ if (PDT && !IsRecalculatingPostDomTree)
+ if (PDT->getNode(DelBB) != nullptr)
+ PDT->eraseNode(DelBB);
----------------
Same as above
================
Comment at: lib/IR/DomTreeUpdater.cpp:319
+ return;
+ if (Ups == UpdateStrategy::Eager) {
+ if (DT)
----------------
I'd put an empty line here too
================
Comment at: lib/IR/DomTreeUpdater.cpp:334
+ return;
+ if (Ups == UpdateStrategy::Eager) {
+ if (DT)
----------------
Same
================
Comment at: lib/IR/DomTreeUpdater.cpp:373
+ OS << "None\n";
+ OS << "UpdateStrategy: ";
+ if (Ups == UpdateStrategy::Eager) {
----------------
Empty line above please
================
Comment at: lib/IR/DomTreeUpdater.cpp:379
+ OS << "Lazy\n";
+ int I = 0;
+ auto printUpdates =
----------------
empty line here
================
Comment at: lib/IR/DomTreeUpdater.cpp:412
+ };
+ if (DT) {
+ OS << "Applied but not cleared DomTreeUpdates:\n";
----------------
Extra line please
================
Comment at: unittests/IR/DomTreeUpdaterTest.cpp:34
+ StringRef FuncName = "f";
+ StringRef ModuleString = "define i32 @f(i32 %i, i32 *%p) {\n"
+ " bb0:\n"
----------------
Consider using c++11 raw string literals here
================
Comment at: unittests/IR/DomTreeUpdaterTest.cpp:54
+ Function *F = M->getFunction(FuncName);
+ ASSERT_NE(F, nullptr) << "Couldn't get function " << FuncName << ".";
+
----------------
Can it to ever fail here?
================
Comment at: unittests/IR/DomTreeUpdaterTest.cpp:101
+ // queued for deletion.
+ SmallVector<BasicBlock *, 1> BBVec;
+ BBVec.push_back(BB3);
----------------
Is it used anywhere?
Repository:
rL LLVM
https://reviews.llvm.org/D48383
More information about the llvm-commits
mailing list