This is an archive of the discontinued LLVM Phabricator instance.

[Analysis][Docs] Parents of loops documentation
ClosedPublic

Authored by baziotis on Feb 20 2020, 2:55 AM.

Details

Summary

Recently I had to use it and although one assumes it returns null if there's no parent loop, I think it helps to doc it.

Diff Detail

Event Timeline

baziotis created this revision.Feb 20 2020, 2:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2020, 2:55 AM
baziotis edited the summary of this revision. (Show Details)Feb 20 2020, 2:59 AM
fhahn added a subscriber: fhahn.Feb 20 2020, 3:20 AM
fhahn added inline comments.
llvm/include/llvm/Analysis/LoopInfo.h
106

Use ///. That's how doxygen comments are defined. It might also be worth clarifying the parent loop relationship in https://llvm.org/docs/LoopTerminology.html.

For the comment here I would suggest to start with a brief first sentence and maybe another one that explain the parent relationship, like

Return the parent loop if it exists or nullptr otherwise.

fhahn added inline comments.Feb 20 2020, 3:54 AM
llvm/include/llvm/Analysis/LoopInfo.h
106

Use ///. That's how doxygen comments are defined. It might also be worth clarifying the parent loop relationship in https://llvm.org/docs/LoopTerminology.html.

Reading this back I realized it might sound a bit harsh. That was not the intention, sorry!

baziotis marked an inline comment as done.Feb 20 2020, 4:45 AM
baziotis added inline comments.
llvm/include/llvm/Analysis/LoopInfo.h
106

No problem, thanks for the review! To change the LoopTerminology.html I change the /llvm/docs/LoopTerminology.rst right?

fhahn added inline comments.Feb 20 2020, 5:01 AM
llvm/include/llvm/Analysis/LoopInfo.h
106

Yep, that would be great!

baziotis updated this revision to Diff 245630.Feb 20 2020, 6:01 AM
baziotis added a reviewer: fhahn.
  • Address comment.
Meinersbur added inline comments.
llvm/docs/LoopTerminology.rst
120–121 ↗(On Diff #245630)

[grammar] "that is it is"

I also don't understand this part of a definition. Does it clarify in what positions a loop can be (it makes it an unusually structured dictionary definition -- I'd expect the first sentence to explain what a "parent loop" is followed by further explanations)? IMHO the LoopInfo being a forest is enough information.

llvm/include/llvm/Analysis/LoopInfo.h
106

Another suggestion: "Return the parent loop or nullptr for topmost loops"

baziotis marked 2 inline comments as done.Feb 20 2020, 2:57 PM
baziotis added inline comments.
llvm/docs/LoopTerminology.rst
120–121 ↗(On Diff #245630)

Ok, thanks for the comment, indeed the definition structure is kind of bad. Regarding the last note, I think it's not stated in the terminology (although it's stated in some parts of LoopInfo.h). Do you think it would be good to add this clarification?
Or maybe a last point in the introduction: "A loop can have only a single parent loop (in which it is entirely contained)". Something like that. Basically just to make obvious that a loop can have only a single parent.

llvm/include/llvm/Analysis/LoopInfo.h
106

Ok. I'm going with "top level" for consistency with the terminology but please comment if I shouldn't.

Meinersbur added inline comments.Feb 21 2020, 9:06 AM
llvm/docs/LoopTerminology.rst
120–121 ↗(On Diff #245630)

I'd expect that tree concepts (node, root, leaf, child, parent, sibling, ancestor, descendent, ...) are known by a majority of programmers, but you should go for what you think is needed for someone who does not know terminology already (i.e. not me). I will look for that it's correct and understandable.

baziotis marked an inline comment as done.Feb 21 2020, 1:55 PM
baziotis added inline comments.
llvm/docs/LoopTerminology.rst
120–121 ↗(On Diff #245630)

Ok great! I think just a small note that loops form a forest of trees (or smth like that) is good. It's mentioned in LoopInfo.h but from the terminology it's not obvious so I think it's beneficial to a newcomer.

baziotis updated this revision to Diff 245993.Feb 21 2020, 2:13 PM
baziotis retitled this revision from [Analysis] getParentLoop() documentation to [Analysis][Docs] Parents of loops documentation.
baziotis edited the summary of this revision. (Show Details)

Address comments. The addition in the terminology might be a little too short.

This revision is now accepted and ready to land.Feb 21 2020, 2:27 PM

LGTM

Thanks! Note: I don't have commit permission, you may commit it whenever you can.

This revision was automatically updated to reflect the committed changes.