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

Mon Feb 17 13:33:57 PST 2020

fhahn added a comment.

>From the description

> Removing support for fixed-size array delinearization resulted in over-pissimization of DependenceAnalysis and reduction of test coverage for both DA and LoopInterchange.

I'm not sure it over-pessimized DA. The change was required for correctness.

================
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
----------------
Meinersbur wrote:
> 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.
I think it might also be beneficial to just extract the interesting cases into a separate test file, rather than adding just a few additional lines to the existing tests.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72178