[PATCH] D65257: Describe resticted form of loops in the new loop terminogy documentation

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: docs/LoopTerminology.rst:138-139
+core to the transforms we generally wish to perform.  Rather than a single
+catch all restriction, we have a number of fine grained predicates which
+can be uses as preconditions for loop transforms.  
+
----------------
reames wrote:
> Meinersbur wrote:
> > I don't get this. Where can these predicates be used? More importantly, which pass ensures the restricted/normal/canonical forms?
> > 
> > I'd not describe a restricted loop forms in terms of an API call. That's what the merthod documentation/doxygen is for. If you are not a frequent user of the Loop class, it is not obvious what it means when e.g. `getLoopLatch()` return nullptr. The naive interpretation is "there is not latch". Rather it should be described as property of the CFG).
> Paragraph A - Many passes bailout with these predicates.  Mostly ensured by LoopSimplify, but other passes help too.
> 
> Paragraph B - I agree that the doxygen should have some of this, but the implications seem like possible overkill.  I could be convinced that this needs to be rewritten as a comment patch though, what do others think?
I think this is specific to pass/analysis implementations and does not belong into a document about loop terminology. Each transformation/pass should document its conditions, which may or may not include some of the ones described here.

I'd put about how to check for frequently used correctness conditions into the loop pass section of WritingAnLLVMPass, if at all.  In this document, I'd describe the loop normal forms we have (LoopSimplify, LoopRotate, IndVarSimplify).


================
Comment at: docs/LoopTerminology.rst:160
+  
+It is generally reasonable to expect the above properties hold for loops of
+interest when a LoopPass runs inside of the LoopPassManager.  Additionally,
----------------
reames wrote:
> Meinersbur wrote:
> > No; `LoopSimplifyPass` must have been added before or `simplifyLoop` run on it manually.
> Correct, but isn't it generally reasonable to assume that has happened by the point a loop pass runs?  I'll admit, my wording was influenced by how things used to work in the old pass manager.  Any suggestions on reframing?
Loop normal forms are not implicit. It must be ensured by the pass (either by calling `simplifyLoop` or in the old pass manager this was possible by depending on e.g. LoopSimplify) or when adding the pass to the LoopPassManager (which might have been done already with some LoopPassManagers in the default pipeline, but this also depends on the position in the pipeline). At least to me this wording gives the impression that there is nothing to do if my pass depends on this condition.


================
Comment at: docs/LoopTerminology.rst:169
+  of this form, but many have been updated, and writing new ones with this
+  restriction is discouraged.
+
----------------
Could you mention the reason why it is not a good idea to transform loops to make this hold?


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

https://reviews.llvm.org/D65257