This is an archive of the discontinued LLVM Phabricator instance.

[Dominators] Introduce DomTreeNodeTraits to allow customization. (NFC)
ClosedPublic

Authored by fhahn on Jan 19 2023, 3:23 PM.

Details

Summary

This patch introduces DomTreeNodeTraits for customization. Clients can implement
DomTreeNodeTraitsCustom to provide custom ParentPtr, getEntryNode and getParent.
There's also a default specialization if DomTreeNodeTraitsCustom is not implemented,
that assume a Function-like NodeT. This is what is used for the existing DominatorTree
and MachineDominatorTree.

The main motivation for this patch is using DominatorTreeBase across all
regions of a VPlan, see D140513.

Diff Detail

Event Timeline

fhahn created this revision.Jan 19 2023, 3:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2023, 3:23 PM
fhahn requested review of this revision.Jan 19 2023, 3:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2023, 3:23 PM
kuhar added inline comments.
llvm/include/llvm/Support/GenericDomTree.h
514–515

Consider moving the graph traits type to the class definition (together with other typedefs) or to a new trait like in D141260.

fhahn updated this revision to Diff 490825.Jan 20 2023, 7:08 AM

Implement and use DomTreeNodeTraits for customization. Clients can implement DomTreeNodeTraitsCustom to provide custom ParentPtr, getEntryNode and getParent. There's also a default specialization if DomTreeNodeTraitsCustom is not implemented, that assume a Function-like NodeT. This is what is used for the existing DominatorTree and MachineDominatorTree.

fhahn retitled this revision from [Dominators] Use GraphTraits::getEntryNode instead of front(). (NFC) to [Dominators] Introduce DomTreeNodeTraits to allow customization. (NFC).Jan 20 2023, 7:10 AM
fhahn edited the summary of this revision. (Show Details)
fhahn updated this revision to Diff 490830.Jan 20 2023, 7:13 AM

Fix build failure.

kuhar added inline comments.Jan 20 2023, 8:01 AM
llvm/include/llvm/Support/GenericDomTree.h
240–244

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> {
  ...
};
}
kuhar added inline comments.Jan 20 2023, 8:07 AM
llvm/include/llvm/Support/GenericDomTree.h
236–237

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.

fhahn updated this revision to Diff 490866.Jan 20 2023, 8:16 AM

Remove DomTreeNodeTraitsCustom and more complicated specialization.

fhahn marked 2 inline comments as done.Jan 20 2023, 8:18 AM
fhahn added inline comments.
llvm/include/llvm/Support/GenericDomTree.h
236–237

The current graph traits only specify getEntryNode(NodePtr) and not getEntryNode(ParentPtr), which is the main reason for doing the direct call here.

240–244

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.

kuhar accepted this revision.Jan 20 2023, 8:27 AM
This revision is now accepted and ready to land.Jan 20 2023, 8:27 AM
fhahn updated this revision to Diff 491143.Jan 22 2023, 3:07 AM
fhahn marked 2 inline comments as done.

rebase to re-trigger precommit tests before merging

This revision was landed with ongoing or failed builds.Jan 22 2023, 12:24 PM
This revision was automatically updated to reflect the committed changes.