[PATCH] D27502: [PM] Edit comments on PM Proxy and utility classes.

[Justin Lebar via Phabricator via llvm-commits]

jlebar added inline comments.


================
Comment at: llvm/include/llvm/IR/PassManager.h:882-883
 
-/// \brief A module analysis which acts as a proxy for a function analysis
-/// manager.
+/// \brief An analysis over a "larger" IR unit that provides access to an
+/// analysis manager over a "smaller" IR unit.
 ///
----------------
mehdi_amini wrote:
> chandlerc wrote:
> > jlebar wrote:
> > > mehdi_amini wrote:
> > > > chandlerc wrote:
> > > > > FWIW, I continue to prefer "outer" and "inner". There is an inherent requirement that the smaller IR units in question be *contained* by the larger units. I'd like words that help indicate that relationship which is a stronger statement than their size IMO.
> > > > +1 with this and all the other naming remarks below.
> > > I agree that containment is the relationship we ideally want to express.
> > > 
> > > However I don't think that "outer" and "inner" are the right words to express that relationship.  Consider two cases:
> > > 
> > > * Every function is contained in a module, and functions together (mostly) comprise a module.  We (or, I, anyway) would not say "a module is outside a function", or "a function's outer module".  I would say "a function's containing module".
> > > 
> > > * Every loop is contained in a function.  But loops are not the primary component of functions.  In this case too I would not say "a function is outside a loop" or "a loop's outer function".  I would say "a loop's containing function".
> > > 
> > > I am happy talking about inner and outer AMs, but I think we're overloading those words in a confusing way when we try to talk about inner/outer IR units.
> > > 
> > > I've thought about this since last night and unfortunately I don't have a concrete suggestion other than "bigger" / "smaller" (written in quotes, as currently in the patch, to indicate the somewhat figurative use of these words).
> > > 
> > > What if we just said
> > > 
> > > > An analysis over a "larger" IR unit that provides access to an analysis manager over a "smaller" IR unit, __which the larger unit must contain__.
> > > 
> > > ?  Is that sufficient?  If not I'll keep pondering this.
> > I see the confusion you're hitting.
> > 
> > For me, "smaller" and "larger" are if anything more confusing though, so I don't think they are an effective way to address the confusion. It just replaces one with another.
> > 
> > I also don't have any concrete suggestions yet, but currently I would leave it as-is until we do have something that isn't such a tradeoff.
> > 
> > I also wonder how much renaming these would help focus on the AMs being "inner" and "outer" rather than the IR units and address the confusion in that way.
> I'm fairly sure that outer and inner are the terms for nesting of loops in the literature, so it is what seems the less confusing to me when extended for other nested IRUnits.
OK, I've rewritten to use "inner" and "outer" -- what do you think?


================
Comment at: llvm/include/llvm/IR/PassManager.h:903
   public:
-    explicit Result(AnalysisManagerT &InnerAM) : InnerAM(&InnerAM) {}
-    Result(Result &&Arg) : InnerAM(std::move(Arg.InnerAM)) {
-      // We have to null out the analysis manager in the moved-from state
-      // because we are taking ownership of the responsibilty to clear the
-      // analysis state.
-      Arg.InnerAM = nullptr;
-    }
+    explicit Result(AnalysisManagerT &AM) : AM(&AM) {}
+    Result(Result &&RHS) : AM(std::move(RHS.AM)) { RHS.AM = nullptr; }
----------------
chandlerc wrote:
> Why the less precise member name?
> 
> There are places where 'AM' is used and is *not* the same as the member, so it seems less ambiguous to give it some more specific name.
Changed back; I don't feel strongly.  In fact I reverted the comment deletion here too.


================
Comment at: llvm/include/llvm/IR/PassManager.h:904
+    explicit Result(AnalysisManagerT &AM) : AM(&AM) {}
+    Result(Result &&RHS) : AM(std::move(RHS.AM)) { RHS.AM = nullptr; }
     Result &operator=(Result &&RHS) {
----------------
chandlerc wrote:
> I dislike using RHS for something that is not a binary operator. A lot of this delta isn't comment-only edits...
Changed back to Arg -- I actually have no idea why I made that change in the first place.

I'd offer to split out the non-comment changes, but in the patch I'm about to upload, they're (all?) gone anyway.


================
Comment at: llvm/include/llvm/IR/PassManager.h:1239
 
-}
+} // namespace llvm
 
----------------
mehdi_amini wrote:
> chandlerc wrote:
> > This isn't common for full-file-scoped namespaces in LLVM. See the last paragraph of http://llvm.org/docs/CodingStandards.html#namespace-indentation
> I believe it is quite common, in llvm/include there are 941 "namespace llvm" and 478 have a closing comment.
I got rid of it so there's no controversy.


https://reviews.llvm.org/D27502