Thanks Alp, Closed by commit r213399 (llvm) and r213400 (clang).
Tyler On Jul 17, 2014, at 6:51 PM, Alp Toker <[email protected]> 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
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
