r213112 - Handle diagnostic warnings in Frontend diagnostic handler.

[Tyler Nowicki]

Thanks Alp,

Closed by commit r213399 (llvm) and r213400 (clang).

Tyler

On Jul 17, 2014, at 6:51 PM, Alp Toker <alp at nuanti.com> wrote:

> 
> On 18/07/2014 03:52, Tyler Nowicki wrote:
>> Hi Alp,
>> 
>> Thanks for the review. I incorporated some of your suggestions. Please review the patches below.
> 
> Thanks Tyler, looks mostly good with a couple of tweaks..
> 
> 
>> 
>>>>     void
>>>>     OptimizationRemarkHandler(const llvm::DiagnosticInfoOptimizationRemark &D);
>>>>     void OptimizationRemarkHandler(
>>>>         const llvm::DiagnosticInfoOptimizationRemarkMissed &D);
>>>>     void OptimizationRemarkHandler(
>>>>         const llvm::DiagnosticInfoOptimizationRemarkAnalysis &D);
>>>> +    void OptimizationWarningHandler(
>>>> +        const llvm::DiagnosticInfoOptimizationWarning &D);
>>> 
>>> I don't get this. The frontend maps backend remarks to frontend diagnostics so why the special case?
>> 
>> This is patch is not another remark. See LLVM commit r213110 which added the warnings to llvm. We produce a warning when an explicitly specified optimization (currently vectorization or interleaving) fails. In clang the user can explicitly specify vectorization, interleaving, and unrolling with a pragma clang loop directive.
> 
> I see the use case now. Good call on the renaming in the updated patch -- the warning flag name makes sense and the type name is better too.
> 
>> 
>> 
>>>> -  Diags.Report(Loc, DiagID) << AddFlagValue(D.getPassName())
>>>> -                            << D.getMsg().str();
>>>> +  // Flag value not used by all optimization messages.
>>>> +  if (D.getPassName())
>>>> +    Diags.Report(Loc, DiagID) << AddFlagValue(D.getPassName())
>>>> +                              << D.getMsg().str();
>>>> +  else
>>>> +    Diags.Report(Loc, DiagID) << D.getMsg().str();
>>> 
>>> Is this change necessary? The existing code had the same functionality without the need for an if/else here AFAICT.
>> 
>> If I changed the implementation of AddFlagValue it might work. The problem is that it is a struct and its constructor expects a StringRef. This is constructed implicitly from the char* returned by getPassName(). But this could be null causing an assertion when building the StringRef.
> 
> Perhaps just use AddFlagValue(D.getPassName() ? D.getPassName() : "") or create a local variable above to avoid duplicating the line.
> 
>> 
>>> I think the plan was to handle this with user-defined diagnostic mappings rather than what's being done here.
>> 
>> What do you mean? Can you elaborate?
> 
> Ah, that comment was about backend diagnostics enabled by -R. Looking at your updated patch I see the mechanism is different here so it doesn't apply.
> 
> 
>> --- test/Driver/optimization-failure.cpp  (revision 0)
>> +++ test/Driver/optimization-failure.cpp    (working copy)
> 
> Not a Driver test. If you put this in 'Misc' alongside the other backend diag tests that'll be fine for now.
> 
> 
>> @@ -0,0 +1,18 @@
>> +// REQUIRES: clang-driver
> 
> The 'clang-driver' predicate is something else, shouldn't be needed here.
> 
>> +// RUN: %clang %s -O3 -gline-tables-only -gcolumn-info -emit-llvm -S -o /dev/null 2>&1 | FileCheck %s
>> +
>> +
>> +// Test verifies optimization failures generated by the backend are handled
>> +// correctly by clang. LLVM tests verify all of the failure conditions.
>> +// CHECK: {{.*}}:12:5: warning: loop not vectorized: failed explicitly specified loop vectorization
> 
> Let's go ahead and test this using the diagnostic verifier (-cc1 -verify) instead of FileCheck.
> 
> See 'test/Misc/backend-stack-frame-diagnostics.cpp' for an example RUN line and expected-warning{{}} directives.
> 
> LGTM with those changes. Feel free to commit both or pass it by me again, whichever you're comfortable with.
> 
> Cheers
> Alp.
> 
> 
> 
>> +
>> +void test_switch(int *A, int *B, int Length) {
>> +  #pragma clang loop vectorize(enable) unroll(disable)
>> +  for (int i = 0; i < Length; i++) {
>> +    switch(A[i]) {
>> +    case 0: B[i] = 1; break;
>> +    case 1: B[i] = 2;   break;
>> +    default: B[i] = 3;
>> +    }
>> +  }
>> +}
> 
> 
> 
>> 
>> Tyler
>> 
>> LLVM:
>> 
>> 
>> 
>> 
>> Clang:
>> 
>> 
>> 
>> 
> 
> -- 
> http://www.nuanti.com
> the browser experts

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140718/0615e715/attachment.html>