[PATCH] D88395: [LoopReroll] Fix rerolling loop with extra instructions

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Thanks for all the effort you put into this and sorry you had to wait so long for somebody to look at this.

I don't think the problem here is the `LoopIncs`. The unrerollable instructions can as well depend on `%iv.next`. You patch would still reroll the following:

  define void @unrerollable_simple([2 x i32]* nocapture %a) {
  entry:
    br label %loop
  
  loop:
    ; base instruction
    %iv = phi i64 [ 0, %entry ], [ %iv.next, %loop ]
  
    ; unrerollable instructions
    %iv.next = add nuw nsw i64 %iv, 1
    %stptrx = getelementptr inbounds [2 x i32], [2 x i32]* %a, i64 %iv.next, i64 0
    store i32 999, i32* %stptrx, align 4
  
    ; extra simple arithmetic operations, used by root instructions
    %plus20 = add nuw nsw i64 %iv, 20
    %plus10 = add nuw nsw i64 %iv, 10
  
    ; root instruction 0
    %ldptr0 = getelementptr inbounds [2 x i32], [2 x i32]* %a, i64 %plus20, i64 0
    %value0 = load i32, i32* %ldptr0, align 4
    %stptr0 = getelementptr inbounds [2 x i32], [2 x i32]* %a, i64 %plus10, i64 0
    store i32 %value0, i32* %stptr0, align 4
  
    ; root instruction 1
    %ldptr1 = getelementptr inbounds [2 x i32], [2 x i32]* %a, i64 %plus20, i64 1
    %value1 = load i32, i32* %ldptr1, align 4
    %stptr1 = getelementptr inbounds [2 x i32], [2 x i32]* %a, i64 %plus10, i64 1
    store i32 %value1, i32* %stptr1, align 4
  
    ; loop-increment and latch
    ;%iv.next = add nuw nsw i64 %iv, 1
    %exitcond = icmp eq i64 %iv.next, 5
    br i1 %exitcond, label %exit, label %loop
  
  exit:
    ret void
  }

To me the problem looks like that the unrerollable store does not belong to any root set and is skipped during verification.

In D88395#2299624 <https://reviews.llvm.org/D88395#2299624>, @kawashima-fj wrote:

> `clang-format` reported a format error `DenseSet<Instruction*>` -> `DenseSet<Instruction *>` (space before `*`). However, other three places in the same function (and many places in other functions) have the same format. Should I change all places? only my change?, or keep all them unchanged?

Use the clang-format suggestion. To goal is to eventually converge to a completely clang-formatted codebase.



================
Comment at: llvm/lib/Transforms/Scalar/LoopRerollPass.cpp:1076
+    collectInLoopUserSet(IV, Exclude, PossibleRedSet, V);
+    for (auto *I : V) {
+      if (I != IV && !isSimpleArithmeticOp(I))
----------------
[style] LLVM's coding style does not make use of [[ https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable | Almost-Always-Auto ]].


================
Comment at: llvm/test/Transforms/LoopReroll/extra_instr.ll:12
+
+;CHECK-LABEL: @rerollable_simple
+;CHECK:       loop:
----------------
[nit] There's usually a space before `CHECK`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88395