[PATCH] D98263: [LoopInterchange] fix tightlyNested() in LoopInterchange legality

[Congzhe Cao via Phabricator via llvm-commits]

congzhe added a comment.

An update: the current trunk code fails the following test (a crash as follows):

`void updateSuccessor(llvm::BranchInst*, llvm::BasicBlock*, llvm::BasicBlock*, std::vector<llvm::cfg::Update<llvm::BasicBlock*>, std::allocator<llvm::cfg::Update<llvm::BasicBlock*> > >&, bool): Assertion 'Changed && "Expected a successor to be updated"' failed.`

  @A = common global [100 x [100 x i64]] zeroinitializer
  @B = common global [100 x i64] zeroinitializer
  
  ;;  for(int i=0;i<100;i++)
  ;;    for(int j=0;j<100;j++)
  ;;      A[j][i] = A[j][i]+k;
  
  ; Crashes with the newest trunk code
  define void @crash(i64 %k, i64 %N, i64 signext %ny) {
  entry:
    br label %for1.header
  
  for1.header:
    %j23 = phi i64 [ 0, %entry ], [ %j.next24, %for1.inc10 ]
    %cmp21 = icmp slt i64 0, %ny
    br label %singleSucc
  
  singleSucc:
    br i1 %cmp21, label %preheader.j, label %for1.inc10
  
  preheader.j:  
    br label %for2
  
  for2:
    %j = phi i64 [ %j.next, %for2 ], [ 0, %preheader.j ]
    %arrayidx5 = getelementptr inbounds [100 x [100 x i64]], [100 x [100 x i64]]* @A, i64 0, i64 %j, i64 %j23
    %lv = load i64, i64* %arrayidx5
    %add = add nsw i64 %lv, %k
    store i64 %add, i64* %arrayidx5
    %j.next = add nuw nsw i64 %j, 1
    %exitcond = icmp eq i64 %j, 99
    br i1 %exitcond, label %for1.inc10, label %for2
  
  for1.inc10:
    %j.next24 = add nuw nsw i64 %j23, 1
    %exitcond26 = icmp eq i64 %j23, 99
    br i1 %exitcond26, label %for.end12, label %for1.header
  
  for.end12:
    ret void
  }

The check from `LoopNest::checkLoopsStructure()` is actually more comprehensive than what was done for `tightlyNested()` in `LoopInterchange.cpp`, for example `checkLoopsStructure()` also considers the inner loop guard blocks, and some other scenarios.  However, the current transformation in `LoopInterchange` may not be able to those complicated scenarios. With the previous check in `tightlyNested()` (as below) which is simpler, it would not pass legality and enter transformation, hence it would not crash.

  // A perfectly nested loop will not have any branch in between the outer and
   // inner block i.e. outer header will branch to either inner preheader and
   // outerloop latch.
   BranchInst *OuterLoopHeaderBI =
       dyn_cast<BranchInst>(OuterLoopHeader->getTerminator());
   if (!OuterLoopHeaderBI)
     return false;
  
   for (BasicBlock *Succ : successors(OuterLoopHeaderBI))
     if (Succ != InnerLoopPreHeader && Succ != InnerLoop->getHeader() &&
         Succ != OuterLoopLatch)
       return false;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98263