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

[Chandler Carruth via Phabricator via llvm-commits]

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


https://reviews.llvm.org/D27502