[PATCH 02/11] Support the new DiagnosticRemarks

[Tobias Grosser]

On 09/06/2014 02:42, Andreas Simbuerger wrote:
> After the ScopDetection is finished, we provide Diagnostic remarks
> for all error entries of each valid Scops parent region.
>
> The goal is to provide developers with useful hints about
> opportunities to increase the size of the detected Scops in their
> code.

Hi Andreas,

very nice that you work on this.

I have one high-level question. It seems you added for each and every 
error a ShortMessage, but some of them contain FIXME or reference 
LLVM-IR constructs that are not useful to the end user. Would it make 
sense to introduce in a first patch only one diagnostic for a single 
error where we know we can provide an appropriate error message and 
introduce a fallback option for the others that informs that other
unspecified problems have occurred. This would avoid ever reporting any
useless/confusing information (or even FIXMEs).

>   include/polly/ScopDetectionDiagnostic.h  | 160 ++++++++++++----
>   lib/Analysis/ScopDetection.cpp           |  47 +++--
>   lib/Analysis/ScopDetectionDiagnostic.cpp | 309 ++++++++++++++++++++++++++++++-

The nice thing about optimization remarks is that they can be nicely 
tested. Would it be possible to add tests?

Specifically, if we start with just a single error, we could immediately 
add a test case. When adding the known errors one-by-one, we could then 
continue to add test cases.

I am asking this because when trying your patches, I had to try a couple 
of problematic files until I found on that reported a remark.

> +
> +  /// @brief Generate a short diagnostic messagedescribing this error.
                                                    ^^^^
Typo.

> +  /// The generated message is suitable for providing additional information
> +  /// to the end user in the form of optimization remarks.
> +  ///
> +  /// @return A short message representing this error.
> +  virtual std::string getShortMessage() const = 0;

This seems not very descriptive. In fact, some of your short messages 
are longer than the normal messages.

What about getEndUserMessage()? I think this is the most important 
feature here. (You could also mention that these messages are not 
supposed to mention any LLVM-IR internal constructs).

> +  // Emit diagnostic remarks for the parents of valid Scops.
> +  // This should help users with increasing the size of valid Scops.

Grammar?

> +  for (const Region *R : ValidRegions) {
> +    const Region *Parent = R->getParent();
> +    if ((Parent != TopRegion) && RejectLogs.count(Parent))
> +      emitRejectionRemarks(F, RejectLogs.at(Parent), LI);
> +  }

Are you saying we call this even if -Rpass was not specified. Did you 
check the compile-time impact of this on a larger file? Or maybe the 
LLVM test-suite?

> +void emitRejectionRemarks(const llvm::Function &F, const RejectLog &Log,
> +                          LoopInfo *LI) {
> +  LLVMContext &Ctx = F.getContext();
> +
> +  const Region *R = Log.region();
> +  Loop *L = LI->getLoopFor(R->getEntry());
> +
> +  if (L) {
> +    DebugLoc DL = L->getStartLoc();
> +    Ctx.diagnose(DiagnosticInfoOptimizationRemark(
> +        DEBUG_TYPE, F, DL,
> +        "The following errors keep this loop from being a Scop."));
> +    for (RejectReasonPtr RR : Log) {
> +      Ctx.diagnose(DiagnosticInfoOptimizationRemark(
> +          DEBUG_TYPE, F, RR->getDebugLoc(), RR->getShortMessage()));
> +    }
> +  }

This seems to assume each region starts with a loop? What about regions 
that contain multiple loops? Or a condition and only then a loop?

What is the rational of only printing errors if we can at the same time 
print a surrounding loop?

> +std::string ReportNonBranchTerminator::getShortMessage() const {
> +  return "(FIXME) Non branch instruction terminates BB";
> +}

I don't think we should emit 'FIXME' to the end user. As proposed above, 
getting a conservative infrastructure in place that in absence of a good 
error message makes clear that there have been further (undescribed) 
errors would be better.

>   std::string ReportNonAffBranch::getMessage() const {
>     return ("Non affine branch in BB '" + BB->getName()).str() + "' with LHS: " +
>            *LHS + " and RHS: " + *RHS;
>   }
>
> +//===----------------------------------------------------------------------===//
> +// ReportNoBasePtr.
> +
> +std::string ReportNoBasePtr::getShortMessage() const {
> +  return "I don't have a base pointer for this array";
> +}

This may need some rewording.

>   std::string ReportNoBasePtr::getMessage() const { return "No base pointer"; }
>
> +//===----------------------------------------------------------------------===//
> +// ReportUndefBasePtr.
> +
> +std::string ReportUndefBasePtr::getShortMessage() const {
> +  return "The base pointer for this array is undefined";
> +}

This does also not seem to make sense for an end-user. Most likely the 
compiler introduced for some reason an undef value. I think it is better 
to admit we do not have a good explanation for this then to emit
a remark that makes little sense to the user. (Most likely this only 
happens for bugpoint reduced test cases, so it is not end-user relevant 
anyway).

>   std::string ReportUndefBasePtr::getMessage() const {
>     return "Undefined base pointer";
>   }
>
> +//===----------------------------------------------------------------------===//
> +// ReportVariantBasePtr.
> +
> +std::string ReportVariantBasePtr::getShortMessage() const {
> +  return "The base address of this array is not invariant inside the loop";
> +}

This is very nice. If we could now also get the name of the array from 
the debug info, we would have a very nice end-user message.

> +std::string ReportSimpleLoop::getShortMessage() const {
> +  return "Try simplifying the loop (-loop-simplify)";
> +}

For a user of clang this message makes little sense.

> -ReportOther::ReportOther() { ++BadOtherForScop; }
> +const DebugLoc &ReportSimpleLoop::getDebugLoc() const {
> +  // FIXME:

What needs to be fixed here?

Cheers,
Tobias