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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

This *is* a much cleaner way to handle ModRefBehavior.
I left some comments inline, some are nits that's up to you if you think they make sense.

I'm more curious if this change impacts compile-time, since it's being pretty general with the iteration over the number of locations possible and I'm not sure how much of that will be inlined.



================
Comment at: llvm/include/llvm/Analysis/AliasAnalysis.h:301
+  /// Get new FunctionModRefBehavior with modified ModRefInfo for Loc.
+  FunctionModRefBehavior withModRef(Location Loc, ModRefInfo MR) const {
+    FunctionModRefBehavior FMRB = *this;
----------------
Naming option: `getWithModRef`?


================
Comment at: llvm/include/llvm/Analysis/AliasAnalysis.h:308
+  /// Get new FunctionModRefBehavior with NoModRef on the given Loc.
+  FunctionModRefBehavior withoutLoc(Location Loc) const {
+    return withModRef(Loc, ModRefInfo::NoModRef);
----------------
similar `getWithoutLoc`?


================
Comment at: llvm/include/llvm/Analysis/AliasAnalysis.h:309
+  FunctionModRefBehavior withoutLoc(Location Loc) const {
+    return withModRef(Loc, ModRefInfo::NoModRef);
+  }
----------------
nit: I'd find this easier to read if it didn't defer to the `withModRef` API, even if it's 2 LoC longer.
```
FunctionModRefBehavior FMRB = *this;
FMRB.setModRef(Loc, ModRefInfo::NoModRef);
return FMRB;
```


================
Comment at: llvm/include/llvm/Analysis/AliasAnalysis.h:352
+  bool onlyAccessesInaccessibleOrArgMem() const {
+    return withoutLoc(ArgMem).withoutLoc(InaccessibleMem).doesNotAccessMemory();
+  }
----------------
Is this `return isNoModRef(getModRef(Other));` ?


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:765
   else if (Call->onlyAccessesInaccessibleMemOrArgMem())
-    Min = FunctionModRefBehavior(Min & FMRB_OnlyAccessesInaccessibleOrArgMem);
+    Min = FunctionModRefBehavior::inaccessibleOrArgMemOnly(MR);
 
----------------
Not sure how much it's worth avoiding the setting `anyLoc`  when it gets overwritten right after.
Suggestion would be to do:
:759 FunctionModRefBehavior Min;

:765
```
else
  Min = FunctionModRefBehavior::anyLoc(MR);
```


================
Comment at: polly/lib/Analysis/ScopBuilder.cpp:1640
+  FunctionModRefBehavior FMRB = AA.getModRefBehavior(CalledFunction);
+
+  if (FMRB.onlyAccessesArgPointees()) {
----------------
Shouldn't the check for does not access memory be kept here?
It's included in the `onlyReadsMemory` below, but it shouldn't add to GlobalReads.

```
if (FMRB.doesNotAccessMemory())
  return true;
```


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

https://reviews.llvm.org/D130896