[PATCH] D93529: [GVN][BasicAA] Enable clobbering in GVN.

[Daniil Fukalov via Phabricator via llvm-commits]

dfukalov marked 2 inline comments as done.
dfukalov added inline comments.


================
Comment at: llvm/lib/Analysis/AliasAnalysis.cpp:950
+    }
+    return AAQI.getClobberOffset({V2, V1});
+  }
----------------
nikic wrote:
> Why is it okay to treat the offset symmetrically? I'd expect the offset either be negative if swapped, or be not defined at all, depending on the contract of this function is supposed to be (probably needs a doc comment).
My thought was to mark "undefined" offset as -1 and store pointers in pairs in "positive offset" order. It was based on GVN ability to use positive offset only for clobbering.
But you're right, it is more effective to store pointers pairs in cache in the same way as it is implemented for AliasCache.


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:1505
 
+  AAQI.VisitedPhi = true;
   // In the recursive alias queries below, we may compare values from two
----------------
nikic wrote:
> Why do you use a separate VisitedPhi flag, rather than checking !VisitedPhiBBs.empty(), as we do elsewhere?
> 
> This kind of flag breaks BatchAA, because it will leak across queries.
Thanks, good catch, missed this leak.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93529