[PATCH] D130896: [AA] Tracking per-location ModRef info in FunctionModRefBehavior (NFCI)
Alina Sbirlea via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Sep 1 11:52:59 PDT 2022
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
More information about the llvm-commits
mailing list