[Analysis][Docs] Parents of loops documentation

[baziotis]

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.

[fhahn]

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]

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]

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]

llvm/include/llvm/Analysis/LoopInfo.h

106

Yep, that would be great!

[baziotis]

• Address comment.

[Meinersbur]

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]

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]

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]

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]

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

[Meinersbur]

LGTM

[baziotis]

In D74890#1887399, @Meinersbur wrote:

LGTM

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

llvm/include/llvm/Analysis/LoopInfo.h

unsigned getLoopDepth() const {
     assert(!isInvalid() && "Loop not in a valid state!");
     unsigned D = 1;
     for (const LoopT *CurLoop = ParentLoop; CurLoop;
          CurLoop = CurLoop->ParentLoop)
       ++D;
     return D;
   }
   BlockT *getHeader() const { return getBlocks().front(); }
+  // Return the parent loop of this loop (i.e. the loop in which
+  // this loop is entirely contained). If there's no such loop (i.e.
+  // this is a top level loop in the function), return null.
   LoopT *getParentLoop() const { return ParentLoop; }
 
   /// This is a raw interface for bypassing addChildLoop.
   void setParentLoop(LoopT *L) {
     assert(!isInvalid() && "Loop not in a valid state!");
     ParentLoop = L;
   }