[PATCH] D67974: [Dominators][AMDGPU] Don't use virtual exit node in findNearestCommonDominator

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added inline comments.


================
Comment at: llvm/lib/CodeGen/MachinePostDominators.cpp:59
+  assert(!Blocks.empty());
+  assert(llvm::all_of(Blocks, [](MachineBasicBlock *B) { return B; }) &&
+         "Invalid block found");
----------------
hliao wrote:
> no need to (expensive) check every BB is not null. findNearestCommonDomniator asserts on both of them are non-null.
This should be a very quick scan over the vector of blocks, not more expensive than the inner check.
The intention was twofold:
1. Document the blocks cannot be nullptr.
2. Fail quickly where the contract is first violated.


================
Comment at: llvm/lib/CodeGen/MachinePostDominators.cpp:67
+    // Stop when the root is reached.
+    if (DT->isVirtualRoot(DT->getNode(NCD)))
+      break;
----------------
hliao wrote:
> the logic here is weird. if virtual root is the NCD, that means there's no real NCD. but, breaking here returns wrong NCD. need to return nullptr explicitly here 
Virtual root is a real PDT node, but not a real basic block. The basic block value is nullptr and this is was will be returned.
I will update it to return nullptr explicitly, if that's easier to follow.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67974/new/

https://reviews.llvm.org/D67974