[PATCH] D72178: [DA] Delinearization of fixed-size multi-dimensional arrays

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/include/llvm/Analysis/DependenceAnalysis.h:932-933
 
-  private:
+    /// Tries to delinearize access function for a fixed size multi-dimensional
+    /// array. Returns true upon success and false otherwise.
+    bool tryDelinearizeFixedSize(Instruction *Src, Instruction *Dst,
----------------
Could you add some more details that it tries to derive the dimensions from GEP and `tryDelinearizeParametricSize` tries to do delinearization? I mean, that's the reason why these functions are separate.


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:3311-3314
+static void
+getIndexExpressionsFromGEP(const GetElementPtrInst *GEP,
+                           SmallVectorImpl<const SCEV *> &Subscripts,
+                           SmallVectorImpl<int> &Sizes, ScalarEvolution &SE) {
----------------
I think this has been split into D73995.


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:3360-3361
 
-  // Below code mimics the code in Delinearization.cpp
-  const SCEV *SrcAccessFn =
-    SE->getSCEVAtScope(SrcPtr, SrcLoop);
-  const SCEV *DstAccessFn =
-    SE->getSCEVAtScope(DstPtr, DstLoop);
+  if (!DisableDelinearizationChecks)
+    return false;
 
----------------
Please add a comment why this is bailing out here.

We could also check for GEP's [[ https://llvm.org/docs/LangRef.html#getelementptr-instruction | inrange ]] modifier and require it unless `DisableDelinearizationChecks` is set.


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:3391-3392
+
+  auto *SrcBasePtr = SrcGEP->getOperand(0);
+  auto *DstBasePtr = DstGEP->getOperand(0);
+  while (auto *PCast = dyn_cast<BitCastInst>(SrcBasePtr))
----------------
[style] LLVM style does not use almost-always-auto


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:3470
   if (!DisableDelinearizationChecks)
-    for (int i = 1; i < size; ++i) {
+    for (size_t i = 1; i < SrcSubscripts.size(); ++i) {
       if (!isKnownNonNegative(SrcSubscripts[i], SrcPtr))
----------------
[style] [[ https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop | Don't evaluate size() in every iteration ]]


================
Comment at: llvm/test/Analysis/DependenceAnalysis/Separability.ll:2
 ; RUN: opt < %s -disable-output "-passes=print<da>" -aa-pipeline=basic-aa 2>&1 \
-; RUN: | FileCheck %s
-; RUN: opt < %s -analyze -basicaa -da | FileCheck %s
+; RUN: -da-assume-inrange-subscripts | FileCheck %s
+; RUN: opt < %s -analyze -basicaa -da -da-assume-inrange-subscripts | FileCheck %s
----------------
bmahjour wrote:
> fhahn wrote:
> > Not sure if it's the best idea to enable this option for a bunch of tests, as we miss test coverage for the code path actually enabled by default.  I think the runs with -da-assume-inrange-subscripts should be an addition, rather than replacing the existing one.
> Unfortunately that will cause dual maintenance. IMHO the loss of coverage suffered as a result of disabling delinearization for the old tests are more concerning than missing coverage on inaccurate results of the current default. Having said that I see your point in that we may want to make sure the current default results don't get any more inaccurate than they already are.
> 
> I really don't see a point in dual maintaining the loop interchange tests though, given that the transform is off by default and there is no point in testing that it fails due to data dependence when there is no real data dependence. Agreed?
I second @fhahn's remark. IMHO regression tests for something unsafe does not happen (here: delinearization without proven safety) are even more important.
Regarding maintenance: I too wish that we had something more maintainable than FileCheck.


================
Comment at: llvm/test/Transforms/LoopInterchange/currentLimitation.ll:1-3
 ; RUN: opt < %s -basicaa -loop-interchange -pass-remarks-missed='loop-interchange' \
-; RUN:   -pass-remarks-output=%t -verify-loop-info -verify-dom-info -S | FileCheck -check-prefix=IR %s
+; RUN:   -da-disable-delinearization-checks -pass-remarks-output=%t -verify-loop-info   \
+; RUN:   -verify-dom-info -S | FileCheck -check-prefix=IR %s
----------------
I think we should continue to also check that interchange does not happen without `-da-disable-delinearization-checks`. Could you add a separate RUN line with another `-check-prefix=`? This also applies to other test cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72178