r213112 - Handle diagnostic warnings in Frontend diagnostic handler.

[Alp Toker]

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