[PATCH] D38725: [LoopUnroll] Clean up remarks for unroll remainder

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

LGTM, please consider moving the test.



================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:504
+        ORE->emit(
+            [&]() { return DiagBuilder() << " with run-time trip count"; });
     }
----------------
dmgreen wrote:
> fhahn wrote:
> > Do I understand correctly that we want to only display this remark in case the following condition is true, so if we successfully unrolled the loop with a run-time trip count (condition at line 428)?
> > 
> > ``` RuntimeTripCount && TripMultiple % Count != 0 && 
> >       UnrollRuntimeLoopRemainder(....) ```
> > 
> > I am probably missing something, but it seems like this code path does not consider the `TripMultiple % Count != 0` from the condition. 
> Currently with -unroll-runtime=true -unroll-remainder (like we have for cortex-m cores), the remainder loop is unrolled. It does this by calling UnrollLoop() from inside UnrollRuntimeLoopRemainder(). When that happens we get two remarks, one from the "outer" UnrollLoop for the original loop, one from the inner UnrollLoop() for the remainder loop. I'm trying to remove that second one for the remainder loop.
> 
> Clang-format moved things around a bit here, but I've just made it so that the ORE is optional, and not passed through to the inner UnrollRuntimeLoopRemainder/UnrollLoop.
Ah yes. Not sure what I was looking at when I left the comment.


================
Comment at: test/Transforms/LoopUnroll/runtime-unroll-remainder.ll:2
 ; RUN: opt < %s -S -loop-unroll -unroll-runtime=true -unroll-count=4 -unroll-remainder -instcombine | FileCheck %s
+; RUN: opt < %s -S -loop-unroll -unroll-runtime=true -unroll-count=4 -unroll-remainder -pass-remarks=loop-unroll 2>&1 | FileCheck %s --check-prefix=REMARKS
 
----------------
I think it would make sense to move the test to test/Transforms/LoopUnroll/loop-remarks.ll?


https://reviews.llvm.org/D38725