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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/include/llvm/Analysis/AliasAnalysis.h:263
+  static FunctionModRefBehavior none() {
+    return FunctionModRefBehavior();
+  }
----------------
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?


================
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;
----------------
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()`?


================
Comment at: llvm/include/llvm/Analysis/AliasAnalysis.h:352
+
+  /// Intersect (in-place) with another FunctionModRefBehavior.
+  FunctionModRefBehavior operator&(FunctionModRefBehavior Other) const {
----------------
ebrevnov wrote:
> I believe "(in-place)" should be removed since it returns new instance.
Right, the "in-place" comments were on the wrong operators...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130896