[PM/AA] Disable the core unsafe aspect of GlobalsModRef in the face of basic changes to the IR such as folding pointers through PHIs, Selects, integer casts, store/load pairs, or outlining.

[chandlerc]

Summary

This leaves the feature available behind a flag. This flag's default
could be flipped if necessary, but the real-world performance impact of
this particular feature of GMR may not be sufficiently significant for
many folks to want to run the risk.

Currently, the risk here is somewhat mitigated by half-hearted attempts
to update GlobalsModRef when the rest of the optimizer changes
something. However, I am currently trying to remove that update
mechanism as it makes migrating the AA infrastructure to a form that can
be readily shared between new and old pass managers very challenging.
Without this update mechanism, it is possible that this still unlikely
failure mode will start to trip people, and so I wanted to try to
proactively avoid that.

There is a lengthy discussion on the mailing list about why the core
approach here is flawed, and likely would need to look totally different
to be both reasonably effective and resilient to basic IR changes
occuring. This patch is essentially the first of two which will enact
the result of that discussion. The next patch will remove the current
update mechanism.

[pete]

LGTM with or without the nitpick. Thanks!

lib/Analysis/IPA/GlobalsModRef.cpp

55 ↗

(On Diff #29758)

Nitpicking, but please make this a hidden option so that people are less likely to use it.

[Gerolf]

lib/Analysis/IPA/GlobalsModRef.cpp

51 ↗

(On Diff #29758)

is -> in

56 ↗

(On Diff #29758)

My preference for default is 'true'. Innocent until proven guilty - with test case counting as a proof. If performance data is in the noise I will switch my preference.

[chandlerc]

Thanks Pete and Gerolf!

Based on Pete's review and some benchmark results Michael Z shared that show very limited impact of GlobalsModRef on performance, I'm submitting this as is. If folks *do* end up seeing regressions, by all means follow up here or on the commit thread and we can try other defaults, etc.

lib/Analysis/IPA/GlobalsModRef.cpp

56 ↗

(On Diff #29758)

Michael Z got spec2k6 (and some other benchmarks) and some other numbers with GlobalsModRef completely disabled and the changes were in the noise, so I'm going with the default of false based on this comment.

However, if you or anyone else gets performance data that is *not* in the noise, just shout, and I'm happy to flip this the other way. =]

llvm/trunk/lib/Analysis/IPA/GlobalsModRef.cpp

 
 STATISTIC(NumNonAddrTakenGlobalVars,
           "Number of global vars without address taken");
 STATISTIC(NumNonAddrTakenFunctions,"Number of functions without address taken");
 STATISTIC(NumNoMemFunctions, "Number of functions that do not access memory");
 STATISTIC(NumReadMemFunctions, "Number of functions that only read memory");
 STATISTIC(NumIndirectGlobalVars, "Number of indirect global objects");
 
+// An option to enable unsafe alias results from the GlobalsModRef analysis.
+// When enabled, GlobalsModRef will provide no-alias results which in extremely
+// rare cases may not be conservatively correct. In particular, in the face of
+// transforms which cause assymetry between how effective GetUnderlyingObject
+// is for two pointers, it may produce incorrect results.
+//
+// These unsafe results have been returned by GMR for many years without
+// causing significant issues in the wild and so we provide a mechanism to
+// re-enable them for users of LLVM that have a particular performance
+// sensitivity and no known issues. The option also makes it easy to evaluate
+// the performance impact of these results.
+static cl::opt<bool> EnableUnsafeGlobalsModRefAliasResults(
+    "enable-unsafe-globalsmodref-alias-results", cl::init(false), cl::Hidden);
+
 namespace {
 /// FunctionRecord - One instance of this structure is stored for every
 /// function in the program.  Later, the entries for these functions are
 /// removed if the function is found to call an external function (in which
 /// case we know nothing about it.
 struct FunctionRecord {
   /// GlobalInfo - Maintain mod/ref info for all of the globals without
   /// addresses taken that are read or written (transitively) by this
if (GV1 || GV2) {
     // If the global's address is taken, pretend we don't know it's a pointer to
     // the global.
     if (GV1 && !NonAddressTakenGlobals.count(GV1))
       GV1 = nullptr;
     if (GV2 && !NonAddressTakenGlobals.count(GV2))
       GV2 = nullptr;
 
     // If the two pointers are derived from two different non-addr-taken
-    // globals, or if one is and the other isn't, we know these can't alias.
+    // globals we know these can't alias.
+    if (GV1 && GV2 && GV1 != GV2)
+      return NoAlias;
+
+    // If one is and the other isn't, it isn't strictly safe but we can fake
+    // this result if necessary for performance. This does not appear to be
+    // a common problem in practice.
+    if (EnableUnsafeGlobalsModRefAliasResults)
       if ((GV1 || GV2) && GV1 != GV2)
         return NoAlias;
 
     // Otherwise if they are both derived from the same addr-taken global, we
     // can't know the two accesses don't overlap.
   }
 
   // These pointers may be based on the memory owned by an indirect global.  If
   // so, we may be able to handle this.  First check to see if the base pointer
   // is a direct load from an indirect global.
AliasResult GlobalsModRef::alias(const MemoryLocation &LocA,
   // These pointers may also be from an allocation for the indirect global.  If
   // so, also handle them.
   if (AllocsForIndirectGlobals.count(UV1))
     GV1 = AllocsForIndirectGlobals[UV1];
   if (AllocsForIndirectGlobals.count(UV2))
     GV2 = AllocsForIndirectGlobals[UV2];
 
   // Now that we know whether the two pointers are related to indirect globals,
-  // use this to disambiguate the pointers.  If either pointer is based on an
-  // indirect global and if they are not both based on the same indirect global,
-  // they cannot alias.
+  // use this to disambiguate the pointers. If the pointers are based on
+  // different indirect globals they cannot alias.
+  if (GV1 && GV2 && GV1 != GV2)
+    return NoAlias;
+
+  // If one is based on an indirect global and the other isn't, it isn't
+  // strictly safe but we can fake this result if necessary for performance.
+  // This does not appear to be a common problem in practice.
+  if (EnableUnsafeGlobalsModRefAliasResults)
     if ((GV1 || GV2) && GV1 != GV2)
       return NoAlias;
 
   return AliasAnalysis::alias(LocA, LocB);
 }
 
 AliasAnalysis::ModRefResult
 GlobalsModRef::getModRefInfo(ImmutableCallSite CS, const MemoryLocation &Loc) {
   unsigned Known = ModRef;
 

llvm/trunk/test/Analysis/GlobalsModRef/aliastest.ll

-; RUN: opt < %s -basicaa -globalsmodref-aa -gvn -S | FileCheck %s
+; RUN: opt < %s -basicaa -globalsmodref-aa -gvn -S -enable-unsafe-globalsmodref-alias-results | FileCheck %s
+;
+; Note that this test relies on an unsafe feature of GlobalsModRef. While this
+; test is correct and safe, GMR's technique for handling this isn't generally.
 
 @X = internal global i32 4    ; <i32*> [#uses=1]
 
 define i32 @test(i32* %P) {
 ; CHECK:      @test
 ; CHECK-NEXT: store i32 7, i32* %P
 ; CHECK-NEXT: store i32 12, i32* @X
 ; CHECK-NEXT: ret i32 7
   store i32 7, i32* %P
   store i32 12, i32* @X
   %V = load i32, i32* %P    ; <i32> [#uses=1]
   ret i32 %V
 }

llvm/trunk/test/Analysis/GlobalsModRef/indirect-global.ll

-; RUN: opt < %s -basicaa -globalsmodref-aa -gvn -instcombine -S | FileCheck %s
+; RUN: opt < %s -basicaa -globalsmodref-aa -gvn -instcombine -S -enable-unsafe-globalsmodref-alias-results | FileCheck %s
+;
+; Note that this test relies on an unsafe feature of GlobalsModRef. While this
+; test is correct and safe, GMR's technique for handling this isn't generally.
 
 @G = internal global i32* null    ; <i32**> [#uses=3]
 
 declare i8* @malloc(i32)
 define void @test() {
   %a = call i8* @malloc(i32 4)
         %A = bitcast i8* %a to i32*
   store i32* %A, i32** @G