[PATCH] D103831: [BasicAA] Handle PHIs without incoming values gracefully

[Daniil Suchkov via Phabricator via llvm-commits]

DaniilSuchkov created this revision.
DaniilSuchkov added reviewers: nikic, dfukalov, mkazantsev.
Herald added a subscriber: hiraditya.
DaniilSuchkov requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

This change fixes a bug introduced by

  commit f6f6f6375d1a4bced8a6e79a78726ab32b8dd879
  Author: Nikita Popov <nikita.ppv at gmail.com>
  Date:   Sun Nov 22 18:23:53 2020 +0100
  
      [BasicAA] Fix BatchAA results for phi-phi assumptions
  
      Change the way NoAlias assumptions in BasicAA are handled. Instead of
      handling this inside the phi-phi code, always initially insert a
      NoAlias result into the map and keep track whether it is used.
      If it is used, then we require that we also get back NoAlias from
      the recursive queries. Otherwise, the entry is changed to MayAlias.
  
      Additionally, keep track of all location pairs we inserted that may
      still be based on assumptions higher up. If it turns out one of those
      assumptions is incorrect, we flush them from the cache.
  
      The compile-time impact for the new implementation is significantly
      higher than the previous iteration of this patch:
      https://llvm-compile-time-tracker.com/compare.php?from=c0bb9859de6991cc233e2dedb978dd118da8c382&to=c07112373279143e37568b5bcd293daf81a35973&stat=instructions
      However, it should avoid the exponential runtime cases we run into
      if we don't cache assumption-based results entirely.
  
      This also produces better results in some cases, because NoAlias
      assumptions can now start at any root, rather than just phi-phi pairs.
      This is not just relevant for analysis quality, but also for BatchAA
      consistency: Otherwise, results would once again depend on query order,
      though at least they wouldn't be wrong.
  
      This ended up both more complicated and more expensive than I hoped,
      but I wasn't able to come up with another solution that satisfies all
      the constraints.
  
      Differential Revision: https://reviews.llvm.org/D91936

Now for empty PHIs, instead of crashing on `assert(hasVal())` in Optional's internals, we'll return NoAlias, as we did before that patch.


https://reviews.llvm.org/D103831

Files:
  llvm/lib/Analysis/BasicAliasAnalysis.cpp
  llvm/test/Transforms/JumpThreading/aa-crash-phi-no-args.ll


Index: llvm/test/Transforms/JumpThreading/aa-crash-phi-no-args.ll
===================================================================
--- llvm/test/Transforms/JumpThreading/aa-crash-phi-no-args.ll
+++ llvm/test/Transforms/JumpThreading/aa-crash-phi-no-args.ll
@@ -1,4 +1,3 @@
-; XFAIL: *
 ; REQUIRES: asserts
 ; RUN: opt -jump-threading -aa-pipeline basic-aa -S -disable-output %s
 ; RUN: opt -passes=jump-threading -aa-pipeline basic-aa -S -disable-output %s
Index: llvm/lib/Analysis/BasicAliasAnalysis.cpp
===================================================================
--- llvm/lib/Analysis/BasicAliasAnalysis.cpp
+++ llvm/lib/Analysis/BasicAliasAnalysis.cpp
@@ -1293,6 +1293,8 @@
 AliasResult BasicAAResult::aliasPHI(const PHINode *PN, LocationSize PNSize,
                                     const Value *V2, LocationSize V2Size,
                                     AAQueryInfo &AAQI) {
+  if (!PN->getNumIncomingValues())
+    return AliasResult::NoAlias;
   // If the values are PHIs in the same block, we can do a more precise
   // as well as efficient check: just check for aliases between the values
   // on corresponding edges.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D103831.350356.patch
Type: text/x-patch
Size: 1147 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210607/75372079/attachment.bin>