[PATCH] D130896: [AA] Tracking per-location ModRef info in FunctionModRefBehavior (NFCI)

[Evgeniy via Phabricator via llvm-commits]

ebrevnov added inline comments.


================
Comment at: llvm/include/llvm/Analysis/AliasAnalysis.h:263
+  static FunctionModRefBehavior none() {
+    return FunctionModRefBehavior();
+  }
----------------
nikic wrote:
> ebrevnov wrote:
> > Default constructor makes unnecessary dependence on internals of ModRefInfo. In particular, it relays on NoModRef being 0. My suggestion is to explicitly use anyLoc(ModRefInfo::NoModRef) here (default constructor can be removed then since this looks to be the only use)
> Removing the default constructor was a bit tricky because we still do need to initialize Data at some point. What I ended up doing is to drop anyLoc() and instead have a proper FunctionModRefBehavior constructor that only takes a ModRefInfo that applies to all locations.
> 
> For now, I have made this constructor explicit, so you have to write `FunctionModRefBehavior(MR)` or similar to use it. An option (regarding your two comments above) would be to make it non-explicit, so that for example `ModRefInfo::Ref` could be implicitly converted to `FunctionModRefBehavior(ModRefInfo::Ref)`. I think if I did this, then there would be no need for methods like `FunctionModRefBehavior::readOnly()`, which has the benefit that there is no need to pick a name for them... It would blur the line between MR and FMRB at bit though. What do you think?
I'm a bit more in favor for the "explicit" version. I think current version is just good enough. Thanks!


================
Comment at: llvm/include/llvm/Analysis/AliasAnalysis.h:308
+  /// Get new FunctionModRefBehavior with NoModRef on the given Loc.
+  FunctionModRefBehavior getWithoutLoc(Location Loc) const {
+    FunctionModRefBehavior FMRB = *this;
----------------
nikic wrote:
> ebrevnov wrote:
> > Do we really need this API? Looks like it is simply  a wrapper and equivalent to getWithModRef(Loc, ModRefInfo::NoModRef) call, which I find more readable. WDYT?
> > 
> I feel like this API does add value -- the typical usage would be as in D130980 to get FMRB without inaccessiblemem or argmem.
> 
> Would it be clearer if this wasn't a generic method, but rather something specific to those, like `FMRB.getWithoutInaccessibleMem()`?
> I feel like this API does add value -- the typical usage would be as in D130980 to get FMRB without inaccessiblemem or argmem.

I personally think this API just makes things harder to understand.
We should either find better name or just use existing API (getWithModRef)

Let's look at the description of getWithoutLoc:

/// Get new FunctionModRefBehavior with NoModRef on the given Loc

This is stated in terms of setting/resetting mod/ref bits for some Loc. I'm having troubles to match this intent with "getWithoutLoc" name. Possible names could be:

resetModRef() or setNoModRef(), etc.

Or just use existing API and pass NoModRef explicitly:
  FMRB.setModRef(Loc, ModRefInfo::NoModRef);




> Currently we have:
> getWithoutLoc(FunctionModRefBehavior::InaccessibleMem);
> 
> Would it be clearer if this wasn't a generic method, but rather something specific to those, like `FMRB.getWithoutInaccessibleMem()`?

I don't think it worths it. 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130896