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

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

Thanks for making the changes @huntergr! I've got a few mostly minor comments so far, but still have to review `findForkedSCEVs`. :)



================
Comment at: llvm/include/llvm/Analysis/LoopAccessAnalysis.h:409
+          DependencySetId(DependencySetId), AliasSetId(AliasSetId) {
+      for (size_t i = 0; i < ScExprs.size(); ++i) {
+        Exprs[i] = ScExprs[i];
----------------
Hi @huntergr, thanks for making the changes to `Exprs`. I just wonder if we should have an assert here that `ScExprs.size() <= 2`?


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:395
+    for (unsigned I = 0; I < Pointers.size(); ++I) {
+      auto Group = CheckingGroups.emplace_back(I, *this);
+      // Try to add a fork to the same group first, otherwise establish
----------------
nit: Is it worth calling `CheckingGroups.emplace_back(I, *this, /*Fork=0*/0)` for clarity here?


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:709
+    Optional<ForkedPointer> FPtr = SE->findForkedPointer(Ptr, L);
+    if (FPtr) {
+      const SCEV *A = FPtr->first;
----------------
nit: This is just a minor comment, but you could remove indentation further here by bailing out early, i.e.

  if (!FPtr)
    return false;

  const SCEV *A =



================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:713
+      LLVM_DEBUG(dbgs() << "LAA: ForkedPtr found: " << *Ptr << "\n");
+      LLVM_DEBUG(dbgs() << "LAA: SCEV1: " << *(FPtr->first) << "\n");
+      LLVM_DEBUG(dbgs() << "LAA: SCEV2: " << *(FPtr->second) << "\n");
----------------
nit: Instead of writing:

  LLVM_DEBUG(dbgs() << "LAA: SCEV1: " << *(FPtr->first) << "\n");

I think you can just write

  LLVM_DEBUG(dbgs() << "LAA: SCEV1: " << *A << "\n");

and same for the second one.



================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:715
+      LLVM_DEBUG(dbgs() << "LAA: SCEV2: " << *(FPtr->second) << "\n");
+      RtCheck.ForkedPtrs[Ptr] = *FPtr;
+      if (isa<SCEVAddRecExpr>(A) && cast<SCEVAddRecExpr>(A)->isAffine() &&
----------------
Is there any danger in setting this before we return true?


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:759
+    auto &DL = L->getHeader()->getModule()->getDataLayout();
+    int64_t Size = DL.getTypeAllocSize(PtrTy->getElementType());
+
----------------
You're introducing a new implicit TypeSize -> uint64_t cast here. Could you rewrite this as:

  int64_t Size = DL.getTypeAllocSize(PtrTy->getElementType()).getFixedSize();



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

https://reviews.llvm.org/D108699