[PATCH] D29989: [BasicAA] Take attributes into account when requesting modref info for a call site

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

This change looks entirely correct, but I think it could simplify greatly by removing the merging logic.  Unless that's needed (test case required), we should simplify.

... actually, after think a bit longer, I'm pretty sure the merging is required for the DSE case.  I'll let you find the test case and explanation to justify why.  :)



================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:747
 
+  ModRefInfo Result = MRI_ModRef;
+
----------------
Given we don't refine this (after suggested change described below) outside of the block we should sink this into the if block.  


================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:770
 
+      // Call doesn't access memory through this operand, so we don't care
+      // if it aliases with Object.
----------------
It looks like most of the merging here is only needed for the fall through logic?  If we remove that, doesn't this simplify greatly.


================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:886
   // The AAResultBase base class has some smarts, lets use them.
-  return AAResultBase::getModRefInfo(CS, Loc);
+  return static_cast<ModRefInfo>(Result & AAResultBase::getModRefInfo(CS, Loc));
 }
----------------
igor-laevsky wrote:
> dberlin wrote:
> > Uh?
> Idea here is to intersect smarts from AAResultBase with the result we've received in this function. I'm actually not certain if this is needed since "AAResults" does intersect results from all alias analyses. Plus AAResultBase currently doesn't have any smarts. So, yes, I'm not sure if we need this operation.
I believe this can be removed.  Or at least, if it can't, it definitely warrants a patch of it's own.  :)


https://reviews.llvm.org/D29989