Add detailed diagnostics to LoopUnroll pass

Hal Finkel hfinkel at
Tue Jan 27 06:07:17 PST 2015

----- Original Message -----
> From: "Michael Zolotukhin" <mzolotukhin at>
> To: "Hal J. Finkel" <hfinkel at>
> Cc: "Commit Messages and Patches for LLVM" <llvm-commits at>
> Sent: Monday, January 26, 2015 4:49:24 PM
> Subject: Add detailed diagnostics to LoopUnroll pass
> Hi,
> This patch adds some diagnostics to loop-unroll pass, triggered by
> -Rpass-analysis=loop-unroll and -Rpass-missed=loop-unroll. Currently
> we only do -Rpass=loop-unroll diagnostics in this pass.

   if (HasUnrollDisablePragma(L)) {

+    emitOptimizationRemarkMissed(Ctx, DEBUG_TYPE, *F, LoopLoc,

+                                 "Unrolling is disabled by pragma.");

Please don't add a trailing period to the messages, I don't believe that's the convention used by the other passes (this applies to all messages).

+                                 "Unrolling is impossible, because the loop "

+                                 "contains non-duplicable instructions.");

Remove comma before 'because'

+    emitOptimizationRemarkMissed(Ctx, DEBUG_TYPE, *F, LoopLoc,

+                                 "Unrolling is disabled, because the loop "

+                                 "contains candidates for inlining.");

Remove comma before 'because'

+  if (TripCount) {

+    if (Count == TripCount &&

+        (Threshold == NoThreshold || UnrolledSize <= Threshold)) {

+      Unrolling = Full;

+      emitOptimizationRemarkAnalysis(Ctx, DEBUG_TYPE, *F, LoopLoc,

+                                     "Will try to completely unroll the loop.");

Saying 'try' here sounds odd. Either the decision has been made or it has not. If not, move the message to some later point. Otherwise, simple state that the loop will be fully unrolled.

+      emitOptimizationRemarkAnalysis(Ctx, DEBUG_TYPE, *F, LoopLoc,

+                                     "Loop cannot be completely unrolled, "

+                                     "because it is too large.");

Remove comma before 'because'

+    emitOptimizationRemarkAnalysis(Ctx, DEBUG_TYPE, *F, LoopLoc,

+                                   "Loop tripcount: " + Twine(TripCount) +

+                                   ". Estimated unrolled size: " +

+                                   Twine(UnrolledSize) +

+                                   ". Threshold: " + Twine(Threshold) + ".");

I'm not sure the size information is appropriate for end users (from their perspective, these are in arbitrary units); I think leaving this in the debug information is sufficient. Please make trip count two words.

+    emitOptimizationRemarkAnalysis(Ctx, DEBUG_TYPE, *F, LoopLoc,

+                                   "Loop tripcount is unknown, only runtime "

+                                   "unrolling is possible.");

Trip count should be two words

+      emitOptimizationRemarkMissed(Ctx, DEBUG_TYPE, *F, LoopLoc,

+                                   "Cannot unroll because loop has a runtime "

+                                   "trip count.");

We also have to be careful here not to produce messaging that users will find confusing. The unroller is run with partial/runtime unrolling disabled many times (iteratively within the inliner-driven CGSCC pass manager), and the user might end up seeing these messages many times for the same loop, which would be confusing, and also confusing because 'disabled' is not really true but rather delayed until the end of the pipeline.

As a result, we should really not say anything about these loops that require runtime/partial unrolling until the "final" run of the unroller at the end of the pipeline.

Thanks again,

> The patch shouldn't change any logic in the pass work, only rearrange
> code a bit and add the reports. Is it ok to commit?
> Thanks,
> Michael

Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory

More information about the llvm-commits mailing list