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

[Graham Hunter via Phabricator via llvm-commits]

huntergr added inline comments.


================
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.
----------------
david-arm wrote:
> 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?
So this is for a RuntimeCheckingPtrGroup, which may have more than two members already; I've only changed it from a SmallVector to a SmallSetVector here in order to avoid duplicate members, since both sides of a forked pointer can be added to the same group. 


================
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
----------------
david-arm wrote:
> Again, does this need to be a `SmallVector` and can it just be `const SCEV *Starts[2];`?
I'll look into it, but it'll make other parts messier since I can't just iterate over the members of the SmallVectors and would need to perform null checks for all those places for the second element.


One solution I thought of (but didn't implement, since I wanted some community feedback on the overall work first) was to create a new SCEVForkedExpr so that we wouldn't need to store two SCEVs here and could just note which fork(s) was represented. It'll take a while to implement that, but might be cleaner. Thoughts?


================
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);
----------------
david-arm wrote:
> Have you rewritten the logic here as a performance improvement, i.e. to avoid calling `Group.addPointer()` after it's already been merged?
Not intentionally, no -- I just replicated the behaviour of the original (break out of the loop if the pointer merged into a group) but considered both potential forks.


================
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
----------------
david-arm wrote:
> Can we also have a test where at least one of the forked pointers is loop-invariant?
We already do; see forked_ptrs_uniform_and_contiguous_forks


We could properly analyze and vectorize that case as well, but I haven't implemented that yet so I'm just testing that it gets rejected for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108699