[PATCH] D99249: [PassManager] Run additional LICM before LoopRotate

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Here's a reduced version of @lebedev.ri's example:

  define void @test(i32* dereferenceable(4) %ptr, i32 %size) {
  entry:                                     
    br label %for.cond
      
  for.cond:
    %iv = phi i32 [ 0, %entry ], [ %iv.next, %for.cont ]
    %cmp = icmp ult i32 %iv, %size
    br i1 %cmp, label %for.cont, label %for.end
  
  for.cont:
    %val = load i32, i32* %ptr, align 4
    %iv.next = add nsw i32 %iv, 1
    call void @use(i32 %val) inaccessiblememonly
    br label %for.cond
  
  for.end:
    ret void
  }
  
  declare void @use(i32)

The problem is that the load is not speculatable due to lack of align attribute. After `-loop-rotate` we have

  define void @test(i32* align 4 dereferenceable(4) %ptr, i32 %size) {
  entry:
    %cmp1 = icmp ult i32 0, %size
    br i1 %cmp1, label %for.cont.lr.ph, label %for.end
  
  for.cont.lr.ph:                                   ; preds = %entry
    br label %for.cont
  
  for.cont:                                         ; preds = %for.cont.lr.ph, %for.cont
    %iv2 = phi i32 [ 0, %for.cont.lr.ph ], [ %iv.next, %for.cont ]
    %val = load i32, i32* %ptr, align 4
    %iv.next = add nsw i32 %iv2, 1
    call void @use(i32 %val) #0
    %cmp = icmp ult i32 %iv.next, %size
    br i1 %cmp, label %for.cont, label %for.cond.for.end_crit_edge
  
  for.cond.for.end_crit_edge:                       ; preds = %for.cont
    br label %for.end
  
  for.end:                                          ; preds = %for.cond.for.end_crit_edge, %entry
    ret void
  }

in which case the `load` is executed unconditionally (if the loop is executed) and can be hoisted. Thus the need for `-licm` after `-loop-rotate`.

Based on @thopre's comments, it seems like lack of align attribute on C++ `this` pointers was also the problem for his case. It's possible that I'm missing some C++ subtlety here, but I believe the lack of align attribute here is just an oversight. At some point, we changed LLVM to no longer assume that pointers without alignment information have natural alignment, and clang was adjusted in D80166 <https://reviews.llvm.org/D80166> to emit align attributes for dereferenceable pointers to compensate for that change. However, the align attributes were added only for reference types, not for `this` pointers. Notably https://github.com/llvm/llvm-project/blob/17800f900dca8243773dec5f90578cce03069b8f/clang/lib/CodeGen/CGCall.cpp#L2310 is missing the align attribute. We're probably missing optimizations not just in LICM because of that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99249