Add detailed diagnostics to LoopUnroll pass

[Michael Zolotukhin]

> On Jan 27, 2015, at 6:07 AM, Hal Finkel <hfinkel at anl.gov> wrote:
> 
> ----- Original Message -----
>> From: "Michael Zolotukhin" <mzolotukhin at apple.com>
>> To: "Hal J. Finkel" <hfinkel at anl.gov>
>> Cc: "Commit Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>
>> 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.
Hmm.. that’s true. Indeed we emit the messages several times for one loop. Now I’m not sure if we need such diagnostics at all. Personally I found it useful, but other users, especially not involved into LLVM development, might have a different opinion on that. Do you think it’s worth adding these messages at all?

Thanks,
Michael
> 
> 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,
> Hal
> 
>> 
>> 
>> 
>> 
>> 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