Add detailed diagnostics to LoopUnroll pass

[Hal Finkel]

----- 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.

 -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