[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:
> > 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.


https://reviews.llvm.org/D27502