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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini 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.
 ///
----------------
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.


https://reviews.llvm.org/D27502