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

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

Hi @huntergr, apologies for delay reviewing this patch! I've not finished reviewing it, but I've left some comments that I have so far. Thanks!



================
Comment at: llvm/include/llvm/Analysis/LoopAccessAnalysis.h:374
+  /// are compatible for a checking group.
+  SmallSetVector<unsigned, 2> Members;
   /// Address space of the involved pointers.
----------------
If we're only ever going to support two forks couldn't we make this simpler by just having:

  unsigned Members[2];

instead or is this a real pain to implement? Or is the idea that we may want to extend the fork to allow more than 2 in future?


================
Comment at: llvm/include/llvm/Analysis/LoopAccessAnalysis.h:395
+    /// all iterations of the loop.
+    SmallVector<const SCEV *, 2> Starts;
+    /// Holds the largest byte address(es) accessed by the pointer throughout
----------------
Again, does this need to be a `SmallVector` and can it just be `const SCEV *Starts[2];`?


================
Comment at: llvm/include/llvm/Analysis/ScalarEvolution.h:444
+/// instruction.
+using ForkedPointer = Optional<std::pair<const SCEV *, const SCEV *>>;
+
----------------
Hi Graham, it feels slightly awkward having to essentially redefine a ForkedPointer as a pair in the same way as `ForkedPtrs` in llvm/include/llvm/Analysis/LoopAccessAnalysis.h. Maybe we could define something like:

  using ForkedPointer = std::pair<const SCEV *, const SCEV *>;

in llvm/include/llvm/Analysis/ScalarEvolution.h that can be used in LoopAccessAnalysis too?


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:187
                        ->getPointerAddressSpace()) {
-  Members.push_back(Index);
+  assert((Fork == 0 || Fork == 1) && "Fork out of range for pointer checking");
+  Members.insert(Index);
----------------
nit: Perhaps you can just write `assert(Fork <= 1 && ...)` since it's unsigned anyway?


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:311
+                                         unsigned Fork) {
+  assert((Fork == 0 || Fork == 1) && "Fork out of range for pointer checking");
   return addPointer(
----------------
nit: Perhaps you can just write `assert(Fork <= 1 && ...)` since it's unsigned anyway?


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:461
+        for (unsigned J = 0; J < NumExprs; ++J)
+          if (!Merged[J])
+            Merged[J] = Group.addPointer(Pointer, *this, J);
----------------
Have you rewritten the logic here as a performance improvement, i.e. to avoid calling `Group.addPointer()` after it's already been merged?


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:698
+  if (!AR) {
+    if (RtCheck.AllowForkedPtrs) {
+      // If we can't find a single SCEVAddRecExpr, then maybe we can find two.
----------------
nit: This is just a suggestion, but you could restructure this a little to remove the extra indentation I think here:

  if (!AR) {
    if (!RtCheck.AllowForkedPtrs)
      return false;
    ...

Not sure if this is better?


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:714
+        RtCheck.ForkedPtrs[Ptr] = *FPtr;
+        if (isa<SCEVAddRecExpr>(A) && cast<SCEVAddRecExpr>(A)->isAffine() &&
+            isa<SCEVAddRecExpr>(B) && cast<SCEVAddRecExpr>(B)->isAffine())
----------------
Is it better to just make a recursive call to hasComputableBounds instead of calling `isAffine` here? We're potentially missing out on future improvements to hasComputableBounds here, and also we're ignoring the possibility of a forked pointer being loop invariant I think.



================
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
----------------
Can we also have a test where at least one of the forked pointers is loop-invariant?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108699