[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/include/llvm/Transforms/IPO/Attributor.h:271
+      second = std::max(second, R.getSize());
+
+    return *this;
----------------
jdoerfert wrote:
> 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.
^ this one we need to handle as well.


================
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:
> jdoerfert wrote:
> > 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
> Right, there is no copy constructor in state, but there is a copy assignment operator which does a shallow copy. I guess it didn't matter because it only got used in places where the state is empty.
> 
> We don't seem to be talking about the same thing here. I totally agree with not using DenseMap, and I am working on using a list instead. My comment was a reply to this granparent comment:
> 
> > We could use Access* here to get stable addresses.
> 
> Where in this specific case do you see the need for stable addresses?
> 
> To me, stable addresses means something like this:
> 
> 
> ```
> SmallVector<Access*> AccessList
> 
> auto *Acc = new Access;
> AccessList.push_back(Acc)
> ```
> 
> Which means now I have to remember to destroy those Access objects later and also worry about deep copies.
> 
> > If the new version requires adequate handling for copy/move constructor, that should be easy to add, no?
> 
> Memory management may be easy, but it is also easily missed. I am quite the fan of coding practices that simply avoid the hassle. For example, I see a commit in git history where the destruction on AccessBins was missed.
> 
> I would much prefer the following:
> 
> ```
> SmallVector<Access> AccessList;
> AccessList.emplace_back({....});
> ```
> 
> Here, AccessList manages the allocation of the Access object and I don't have to worry about destruction and copying.
> 
> This is what "never use new" means. To repeat, it has nothing to do with the cost of DenseMap. It also has nothing to do with custom Allocators. The point is that application programmers should refrain from using the new operator, and instead rely on containers to take care of the allocation. The simplest such container is unique_ptr, for example. All containers take Allocators as template arguments, so the question of which Allocator to use is completely orthogonal. "never use new" is a code hygiene concept, not a performance concept.
> 
> So do you really see a reason to have stable pointers to Access objects? Otherwise I would much prefer keeping them directly in a SmallVector.
I'm happy with alternatives. Stable addresses would have allowed to do avoid all the indices and double lookups, though, that is not by itself a problem. I thought below it would be easier if we could store Access*, if you think we don't need to, that's generally OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136526