[PATCH] D136514: [AA][Intrinsics] Add separate_storage assumptions.

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

This looks basically fine to me. My one remaining concern here is compile-time impact. Adding a new SeparateStorageAliasAnalysis pass does add some overhead even if these assumptions are not used (the last two patches add about 0.2% overhead). I think we might be better off just folding this code into BasicAA at an appropriate place, which also saves a good bit of boilerplate.



================
Comment at: llvm/docs/LangRef.rst:2514
 
+
 Operand bundles on an :ref:`llvm.assume <int_assume>` allows representing
----------------
Spurious newline.


================
Comment at: llvm/docs/LangRef.rst:2534
       "<tag>"([ <holds for value> [, <attribute argument>] ])
+    
 
----------------
Spurious indent.


================
Comment at: llvm/docs/LangRef.rst:2602
 
+
 .. _ob_preallocated:
----------------
Spurious newline


================
Comment at: llvm/lib/Analysis/SeparateStorageAliasAnalysis.cpp:58
+
+  for (auto &AssumeVH : AC.assumptions()) {
+    if (!AssumeVH)
----------------
davidtgoldblatt wrote:
> nikic wrote:
> > Hm, I wonder if we can use assumptionsFor() here?
> I think eventually we should be able to, but there's two things that make this tricky:
> - I think the AssumptionCache assumes that the only value that matters for `assumptionsFor` is the first one (because of the param attribute background?), which would introduce a weird asymmetry in how we can use it.
> - We care about the comparison after a getUnderlyingObject call, but the AssumptionCache doesn't trace through GEPs in findAffectedValues.
> (plausibly other issues; I checked that naive attempts at assumptionsFor didn't work, but didn't dig as much into the why).
> 
> I think the right thing to do eventually is to rewrite the hints during InstCombine (which should fix issue 2 as well as just speeding things up), but figured it didn't make sense to add extra complication to the AssumptionCache to fix 1 until that was done.
My thinking was that we add the underlying object as the affected value in the assumption cache, but your suggestion to strip that away in InstCombine does sound better.


================
Comment at: llvm/test/Analysis/SeparateStorageAliasAnalysis/alias_test.ll:1
+; RUN: opt < %s -aa-pipeline=separatestorage-aa,basic-aa -gvn -enable-separate-storage-aa -S | FileCheck %s
+
----------------
I don't think you need to specify aa-pipeline here, it's in the default pipeline.


================
Comment at: llvm/test/Analysis/SeparateStorageAliasAnalysis/alias_test.ll:3
+
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
+
----------------
Data layout not necessary.


================
Comment at: llvm/test/Analysis/SeparateStorageAliasAnalysis/alias_test.ll:7
+
+; Test basic queries.
+
----------------
It would be preferable to make this an `aa-eval` test, to test AA directly, rather than a pass using it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136514