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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:832
+  // information in OffsetAndSize in the Access object.
+  SmallVector<AAPointerInfo::Access, 4> AccessList;
+  OffsetBinsTy OffsetBins;
----------------
sameerds wrote:
> sameerds wrote:
> > jdoerfert wrote:
> > > We could use Access* here to get stable addresses.
> > We could, but then we have to worry about memory allocation on the stack. In fact, I am still wondering about the copy constructor for PointerInfo::State. It seems to make a shallow copy of AccessBins. Doesn't that result in a double free? 
> > 
> > If we keep a list of Access*, instead of a list of Access, it looks like more code with no clear benefit. TBH, I am a big fan of "never use new". For now, the unsigned index into the list of Access is sufficient for its very local use. If it needs to be exposed to clients, maybe could wrap it in a custom iterator instead of exposing stable pointers?
> > 
> > Of course I am open to using a list of Access* if you think that is beneficial.
> s/stack/heap
"never use new" is not a good argument. For one, we have a bump allocator for the Attributor so the "cost" argument doesn't apply at all. Second, even if we don't use the bump allocator, new is not inherently costly. DenseMaps of DenseMaps (or similar nested structures) can hover grow fast. They also will inherently use new, so all you do here is to hide the heap allocations and cause more of them.

> We could, but then we have to worry about memory allocation on the stack. In fact, I am still wondering about the copy constructor for PointerInfo::State. It seems to make a shallow copy of AccessBins. Doesn't that result in a double free?

As far as I can tell, PpinterInfo::State did not have a copy constructor. The move constructor did not cause double free problems as it cleared the other access bin after moving the content. If the new version requires adequate handling for copy/move constructor, that should be easy to add, no?

> Of course I am open to using a list of Access* if you think that is beneficial.

As I said, I had mixed experience with nested maps. This is not exactly the same case but the first one I could find: https://reviews.llvm.org/rG14cb0bdf2b6ca0b7befbb07fe9f73dad5786f59b


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136526