[PATCH] D18939: [AliasSetTracker] Correctly handle changing size of an entry

[Hal Finkel via llvm-commits]

hfinkel added inline comments.

================
Comment at: lib/Analysis/AliasSetTracker.cpp:278
@@ +277,3 @@
+    // If the size changed, we may need to merge several alias sets.
+    // Use findAliasSetForPointer for its merging side effect.
+    // Note that we can *not* return the result of findAliasSetForPointer
----------------
mkuper wrote:
> hfinkel wrote:
> > Saying "Use findAliasSetForPointer for its merging side effect." implies that findAliasSetForPointer does other things too. If it did, it would be reasonable to split the behavior. However, merging together all alias sets aliasing the provided pointer is exactly what findAliasSetForPointer does. I think this is worth saying. Instead of:
> > 
> >   // Use findAliasSetForPointer for its merging side effect. 
> > 
> > how about:
> > 
> >   // Merging together all alias sets which alias a particular pointer is exactly what findAliasSetForPointer does.
> I agree it doesn't make sense to split the behavior out, since it's basically all it does - hence the comment, otherwise I would have just split it out.  :-)
> The problem is that findAliasSetForPointer is not really a good name for what it does. But I don't have any better ideas, "mergeAliasSetsForPointer" that returns the resulting alias set doesn't look much better.
> 
> Anyway, this is bike-shedding, if you prefer just changing the comment, I'm ok with that.
> Let me know what you think.
Indeed :-) -- I also could not think of a better name. Thus I suggested just changing the comment. 

FWIW, I do like "mergeAliasSetsForPointer" better than "findAliasSetForPointer".

================
Comment at: lib/Analysis/AliasSetTracker.cpp:282
@@ +281,3 @@
+    // is NoAlias, findAliasSetForPointer(undef, ...) will not find the
+    // the right set for undef, even if it exists.
+    if (Entry.updateSizeAndAAInfo(Size, AAInfo))
----------------
mkuper wrote:
> hfinkel wrote:
> > What is the right alias set for undef?
> An alias set that contains only undef - it can never be merged with anything else, since undef aliases nothing.
> 
> I guess this makes sense - getAliasSetForPointer() needs to return *something*, and returning a set that contains only undef means we'll get an alias set that aliases nothing else, as expected. I guess this isn't as precise as we'd idally like, because it means we merge "different" undefs, which should not alias each-other.
> 
> BTW, this isn't necessarily a problem only for undef. The issue is that alias(X,X) apparently doesn't have to hold, in general.
> An alias set that contains only undef - it can never be merged with anything else, since undef aliases nothing.

> I guess this makes sense - getAliasSetForPointer() needs to return *something*, and returning a set that contains only undef means we'll get an alias set that aliases nothing else, as expected. I guess this isn't as precise as we'd idally like, because it means we merge "different" undefs, which should not alias each-other.

Wouldn't it make the most sense for each undef to get its own set? I don't see why we should merge them since different undefs don't mutually alias.

> BTW, this isn't necessarily a problem only for undef. The issue is that alias(X,X) apparently doesn't have to hold, in general.

Really? That seems wrong.



http://reviews.llvm.org/D18939