[PATCH 02/11] Support the new DiagnosticRemarks

Tue Jun 10 04:06:07 PDT 2014

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hey thanks for reviewing this. As you have noticed, the patch is in a
very raw state so far. Before going into refining the patch, I need an
oppinion on the message format.
How do we want to give the clang user hints on what he can do to
increase the size of the Scops?

1)
 "You should remove that alias here..".
 "Try eliminating that parameter here...".

or more like a simple status dump:
2)
 "This access is not affine ..."

Variant (1) looks harder to get the remark right in terms of
additional diagnostic info required (e.g., parameters that cause
non-affinity).

Variant (2) is less precise but easier.

On 06/10/2014 08:58 AM, Tobias Grosser wrote:
> 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).
> 

Yep, I hoped that during review we might come up with better short
messages than the ones I came up with and useful ones for the FIXMEs ;-).

Reporting something like 'Unspecified Error' seems reasonable. One
might argue that giving the 'Generic/Unspecified error' is about as
useful as the FIXME stuff'. I change it in the commit.

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

It would be. But looks like a big pile of work ;-) *sigh*.

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

As soon as I get around to it I'll add one simple test for each
reported error.

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

Grmpf. ;-)

>> +  /// 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).

OK.

> 
>> +  // 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?

No, I have not tested the compile-time impact. Unfortunately we do not
have access to the clang command line flags in opt afaik. I could
check if the flag is enabled in opt easily but not in clang.

So either we emit them all the time (1), or emit them as soon as the
user passes the polly-detect-track-failures option (2).

(1) might be bad in terms of compile-time.
(2) is bad, if the clang user has to know that he needs
polly-detect-track-failures in addition to -Rpass=".*"

Would be easier to have a way of checking that the remarks are
requested by the user.

> 
>> +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?

Mhhh. Actually that is a bit short-sighted you're right. After giving
it a second thought it shouldn't be restricted at all und just print
something like: "The size of the detected Scop can be increased by
fixing the following errors."

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

Yep sure.

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

Ok.

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

Alright. Or maybe something like "Could not find the array for the
access?" ?

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

I'm not an expert on using the DebugInfo metadata, do you happen to
know where I can find a small example on how to retreive it properly?

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

What would be a better wording?

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

Well, the debug location is empty. But I will take care of that
in the next revision anyway.

> 
> Cheers, Tobias
> 

- -- 
Andreas Simbürger
University of Passau - Programming Group
http://www.infosun.fim.uni-passau.de/cl/staff/simbuerger/

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQEcBAEBAgAGBQJTluafAAoJELS7a/6Ti2HYcYAIALVWCrGWq+tiMRnHh6dBYK4A
+shaw7wiUiSc6+BT6p9PqGMRvGh7V9z7lpC3UaEkk1A3GcYm3WVpOdiD++r1iDpq
ZSwqN6uIFwIGaPK5iCP+WjPpLYie4Kx6wZ8clCgqwPs/1k48LW8Qn68tNPTf8r6C
tf74xeuR64EZOEeBIZeUf4B/5rIn9qfbscE6Mkd0JAhUm03X16g5j99EnD2k3DYQ
8fHEj8z1knDH/gUoHvKveDSMYwW3Rox1F12zzG6HLeNRicT8n5ihcDJ9w/9EEkPn
KKjHfZv8Ub45DbrqORkFUQ0r4l4jJPqVqb17p2GWr4WzzoCsOf8c8jCBJTK/kVc=
=NCMG
-----END PGP SIGNATURE-----