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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour marked 2 inline comments as done.
bmahjour added inline comments.


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:113
                 cl::desc("Try to delinearize array references."));
-static cl::opt<bool> DisableDelinearizationChecks(
-    "da-disable-delinearization-checks", cl::init(false), cl::Hidden,
-    cl::ZeroOrMore,
-    cl::desc(
-        "Disable checks that try to statically verify validity of "
-        "delinearized subscripts. Enabling this option may result in incorrect "
-        "dependence vectors for languages that allow the subscript of one "
-        "dimension to underflow or overflow into another dimension."));
+static cl::opt<bool> AssumeInRangeSubscripts(
+    "da-assume-inrange-subscripts", cl::init(false), cl::Hidden, cl::ZeroOrMore,
----------------
fhahn wrote:
> What is the rational for renaming the option? Enabling the option is unsafe and the new name makes it sound a lot more harmless than the original one. Also I think we should retain the wording in the description stating why it is unsafe.
I renamed it because the option is now being used to control more than just the delinearization validity checks. Now it also controls whether fixed size array delinearization happens or not.  I can add the words of caution from the old description back.


================
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
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72178