Add detailed diagnostics to LoopUnroll pass

[Michael Zolotukhin]

> On Jan 27, 2015, at 11:44 AM, Hal Finkel <hfinkel at anl.gov> wrote:
> 
> ----- Original Message -----
>> From: "Michael Zolotukhin" <mzolotukhin at apple.com>
>> To: "Hal Finkel" <hfinkel at anl.gov>
>> Cc: "Commit Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>
>> Sent: Tuesday, January 27, 2015 1:36:14 PM
>> Subject: Re: Add detailed diagnostics to LoopUnroll pass
>> 
>> 
>>> 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?
> 
> I recommend leaving them out for now; we can revisit this later if we'd like.
Okay, that makes sense. Thank you for the comments and review!

Michael
> 
> -Hal
> 
>> 
>> 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
>> 
>> 
> 
> -- 
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150127/b9f29816/attachment.html>