[PATCH] D142162: [Dominators] Introduce DomTreeNodeTraits to allow customization. (NFC)

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 2 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/include/llvm/Support/GenericDomTree.h:236-237
+
+  static NodeT *getEntryNode(ParentPtr Parent) { return &Parent->front(); }
+  static ParentPtr getParent(NodePtr BB) { return BB->getParent(); }
+};
----------------
kuhar wrote:
> nit: In the default implementation, it also seems reasonable to me to go through the related graph traits methods. But since both will end up calling these, either way (i.e., direct calls vs. graph traits) seems fine to me.
The current graph traits only specify `getEntryNode(NodePtr)` and not `getEntryNode(ParentPtr)`, which is the main reason for doing the direct call here.


================
Comment at: llvm/include/llvm/Support/GenericDomTree.h:240-244
+/// DomTreeNode traits specialization to use DomTreeTraitsCustom if it is
+/// defined for NodeT.
+template <typename NodeT>
+struct DomTreeNodeTraits<
+    NodeT, std::void_t<typename DomTreeNodeTraitsCustom<NodeT>::ParentPtrTy>> {
----------------
kuhar wrote:
> Do we need this? I'd think one could just directly specialize `DomTreeNodeTraits` for their type without the need for the second template parameter and `DomTreeNodeTraitsCustom`:
> 
> ```
> namespace llvm {
> template <>
> struct DomTreeNodeTraits<MyNodeType> {
>   ...
> };
> }
> ```
> 
Yeah, good point! My original thought. was to not require `DomTreeNodeTraitsCustom` to specify everything (e.g. `NodeType`), but it makes things more complicated than necessary.

I remove `DomTreeNodeTraitsCustom` and updated the `DomTreeNodeTraits` specialization in VPlanDominatorTree.h to specify everything.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142162