[PATCH] D136526: [AAPointerInfo] refactor how offsets and Access objects are tracked

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Test impact looks good now. I have two issues with the patch but we can resolve both. One is the update of the OAS which needs to also trigger an update of the access kind and value, e.g., if we loose precise information we cannot keep the MUST kind and value around. Second, we should probably avoid nested DenseMaps, I tried to propose some alternatives.



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:271
+      second = std::max(second, R.getSize());
+
+    return *this;
----------------
At this point we need to signal the access it is a May not Must access anymore if the offset or size changed from something that was not unassigned, right? We also need to give up on the value I suppose.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:273
+    return *this;
+  }
+
----------------
Not super happy about first and second. Should we wrap it in `setOffset` `setSize` ?


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:5099
 
+    AA::OffsetAndSize OAS = AA::OffsetAndSize::getUnassigned();
+
----------------
Documentation, for all of these. See the surrounding code.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:832
+  // information in OffsetAndSize in the Access object.
+  SmallVector<AAPointerInfo::Access, 4> AccessList;
+  OffsetBinsTy OffsetBins;
----------------
We could use Access* here to get stable addresses.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:835
+  DenseMap<const Instruction *, DenseMap<const Instruction *, unsigned>>
+      InstTupleMap;
+
----------------
I'm not sure nested DenseMaps are a good idea. I think it usually ends up using way too much memory.
Let's try to use an encoding without it.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:878
 
-    // Now that we have an offset and size, find all overlapping ones and use
-    // the callback on the accesses.
-    return forallInterferingAccesses(OAS, CB);
+    return Changed;
   }
----------------
This looks like we could map instructions to a list of OAS instead. Or directly the Access*?


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:896
+  auto &LocalMap = InstTupleMap[RemoteI];
+  auto [AccIt, AccInserted] = LocalMap.try_emplace(&I, AccessList.size());
+  auto Index = AccIt->getSecond();
----------------
Also here, if we have `RemoteI -> list<Access*>` we should be fine, no?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136526