[PATCH] D76132: [LoopUnrollAndJam] Changed safety checks to consider more than 2-levels loop nest.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/test/Transforms/LoopUnrollAndJam/dependencies.ll:1
-; RUN: opt -basicaa -loop-unroll-and-jam -allow-unroll-and-jam -unroll-and-jam-count=4 < %s -S | FileCheck %s
-; RUN: opt -aa-pipeline=basic-aa -passes='unroll-and-jam' -allow-unroll-and-jam -unroll-and-jam-count=4 < %s -S | FileCheck %s
+; RUN: opt -da-disable-delinearization-checks -basicaa -loop-unroll-and-jam -allow-unroll-and-jam -unroll-and-jam-count=4 < %s -S | FileCheck %s
+; RUN: opt -da-disable-delinearization-checks -aa-pipeline=basic-aa -passes='unroll-and-jam' -allow-unroll-and-jam -unroll-and-jam-count=4 < %s -S | FileCheck %s
----------------
Whitney wrote:
> dmgreen wrote:
> > Why is this now using da-disable-delinearization-checks, and why have some of these existing tests been changed to use constant size arrays?
> `-da-disable-delinearization-checks` is added to more accurately delinearization of fixed-size multi-dimensional arrays. See 
> https://reviews.llvm.org/D72178 more detail explaination. 
> 
> > why have some of these existing tests been changed to use constant size arrays
> 
> They were originally testing single dimensional arrays, which may not be ideal for testing sub-sub portion of code. 
> -da-disable-delinearization-checks is added to more accurately delinearization of fixed-size multi-dimensional arrays. 

Well, it returns more optimistic results at the loss of soundness IIRC. I think we should definitely keep test coverage without -da-disable-delinearization-checks. This is the common case, we should definitely handle correctly. Ideally we would have multi-dimensional tests that *do not* need -da-disable-delinearization-checks. IIRC constant loop bounds might help with that. 

IMO tests that really require -da-disable-delinearization-checks should be additional, maybe in a separate file.


================
Comment at: llvm/test/Transforms/LoopUnrollAndJam/dependencies.ll:401
 ; CHECK: %j.1 = phi
-define void @sub_sub_eq(i32* noalias nocapture %A, i32 %N, i32* noalias nocapture readonly %B) {
 entry:
----------------
Whitney wrote:
> This test was orignally testing
> ```
> for i
>   for j
>     A[i]
>     A[i]
> ```
> 
> I think it actually want to test the code for sub with sub with the dependence distance of the inner loop is eq.
> ```
> for i
>   for j
>     A[i][j]
>     A[i+1][j]
> ```
 FWIW I think it would be better to keep the original test as is and add the new case as an additional test, as they seem to test different scenarios.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76132