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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

I think this looks good. Fixes an error and I hope doesn't introduce new ones.
Could you, in a follow up, add some tests for the multi access per instruction support you introduced now?



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:214
 /// uncertainty and check for overlapping accesses.
-struct OffsetAndSize : public std::pair<int64_t, int64_t> {
-  using BaseTy = std::pair<int64_t, int64_t>;
-  OffsetAndSize(int64_t Offset, int64_t Size) : BaseTy(Offset, Size) {}
-  OffsetAndSize(const BaseTy &P) : BaseTy(P) {}
-  int64_t getOffset() const { return first; }
-  int64_t getSize() const { return second; }
-  static OffsetAndSize getUnknown() { return OffsetAndSize(Unknown, Unknown); }
-  static OffsetAndSize getUnassigned() {
-    return OffsetAndSize(Unassigned, Unassigned);
-  }
+struct OffsetAndSize {
+  int64_t Offset = Unassigned;
----------------
tschuett wrote:
> Would a class with public and private make this safer to use?
We use it as a POD, basically a pair with helper functions and nicer names. I doubt there is much value in private members and public setters.


================
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;
   }
----------------
sameerds wrote:
> jdoerfert wrote:
> > This looks like we could map instructions to a list of OAS instead. Or directly the Access*?
> Should we do this in a separate change? Latest diff addresses pretty much all the other comments.
This doesn't need addressing per se. Let's keep it as is for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136526