This is an archive of the discontinued LLVM Phabricator instance.

[DomTree] findNearestCommonDominator: assert the nodes are in tree
ClosedPublic

Authored by MaskRay on Oct 2 2020, 3:02 PM.

Details

Summary

i.e. they cannot be unreachable from the entry (which usually indicate usage errors).

Diff Detail

Event Timeline

MaskRay created this revision.Oct 2 2020, 3:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 2 2020, 3:02 PM
MaskRay requested review of this revision.Oct 2 2020, 3:02 PM
kuhar added a comment.Oct 3 2020, 3:52 PM

Does this break any existing code?

llvm/include/llvm/Support/GenericDomTree.h
483–484

Maybe assert each node separately and assertion message explaining what this means?

483–494

I both NodeA and NodeB have to be non-null before the loop, and we unconditionally dereference NodeA at the return statement, then one of the three must be off: (1) the assertion, (2) the loop condition, (3) the return.

MaskRay updated this revision to Diff 296014.Oct 3 2020, 5:15 PM

Delete an unneeded loop condition

MaskRay marked 2 inline comments as done.Oct 3 2020, 5:16 PM
MaskRay added inline comments.
llvm/include/llvm/Support/GenericDomTree.h
483–494

Good catch. I have deleted the redundant check.

check-llvm check-clang passed.

MaskRay marked an inline comment as done.Oct 3 2020, 5:21 PM
kuhar accepted this revision.Oct 4 2020, 1:56 PM
This revision is now accepted and ready to land.Oct 4 2020, 1:56 PM
kuhar requested changes to this revision.Oct 4 2020, 2:02 PM

One remaining thing: the function documentation needs to be updated:

If there is no such block then return nullptr.

This is obsolete, and it may be worth mentioning that A and B must have tree nodes.

Or actually, would it make sense to just make the function accept tree nodes instead of basic blocks?

This revision now requires changes to proceed.Oct 4 2020, 2:02 PM
MaskRay updated this revision to Diff 296078.Oct 4 2020, 2:16 PM

Update comment

One remaining thing: the function documentation needs to be updated:

If there is no such block then return nullptr.

This is obsolete, and it may be worth mentioning that A and B must have tree nodes.

Fixed.

Or actually, would it make sense to just make the function accept tree nodes instead of basic blocks?

There are 21 references of the function. Many don't have tree nodes around. Changing to tree nodes can inconvenience many call sites.

kuhar accepted this revision.Oct 4 2020, 2:19 PM

OK, thanks for looking into this.

This revision is now accepted and ready to land.Oct 4 2020, 2:19 PM
This revision was landed with ongoing or failed builds.Oct 4 2020, 3:38 PM
This revision was automatically updated to reflect the committed changes.