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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

I think it would be good to split this up into the 3 distinct parts you mention in the description:

1. re-introduce the checks
2. rename option
3. improve logic



================
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,
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72178