[PATCH] D108699: [LAA] Analyze pointers forked by a select

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D108699#3148674 <https://reviews.llvm.org/D108699#3148674>, @david-arm wrote:

> LGTM! Thanks for making all the changes @huntergr and adding the tests. This patch is low risk at the moment with the flag being disabled currently. At some point once there have been performance investigations to prove there are no regressions we can then enable this by default.



================
Comment at: llvm/include/llvm/Analysis/LoopAccessAnalysis.h:20
 #include "llvm/Analysis/LoopAnalysisManager.h"
+#include "llvm/Analysis/ScalarEvolution.h"
 #include "llvm/Analysis/ScalarEvolutionExpressions.h"
----------------
Is this needed? Other SCEV types are forward-declared below, so ScalarEvolution.h doesn't need to be included.


================
Comment at: llvm/include/llvm/Analysis/LoopAccessAnalysis.h:334
+  /// pointer, with index \p Index in RtCheck and using \p Fork (0 or 1) if
+  /// this represents a forked pointer.
+  RuntimeCheckingPtrGroup(unsigned Index, RuntimePointerChecking &RtCheck,
----------------
Could we use a bool for `Fork` if it only has 2 possible values?


================
Comment at: llvm/include/llvm/Analysis/ScalarEvolution.h:769
+  /// a select instruction.
+  Optional<ForkedPointer> findForkedPointer(Value *V, const Loop *L);
+
----------------
This is only used by LAA. Is there a reason this needs to be part of `ScalarEvolution`?


================
Comment at: llvm/test/Transforms/LoopVectorize/forked-pointers.ll:1
+; RUN: opt -loop-accesses -analyze -enable-new-pm=0 %s 2>&1 | FileCheck %s --check-prefix=NO-FORKED-PTRS
+; RUN: opt -disable-output -passes='require<scalar-evolution>,require<aa>,loop(print-access-info)' %s 2>&1 | FileCheck %s --check-prefix=NO-FORKED-PTRS
----------------
the tests running `-loop-accesses` should be in `llvm/test/Analysis/LoopAccessAnalysis`.

Also, could you pre-commit the tests and update the diff here to show only the difference? That way it is a bit easier to see the impact in the diff.


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

https://reviews.llvm.org/D108699