<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Thanks Alp,<div><br></div><div>Closed by commit r213399 (llvm) and r213400 (clang).</div><div><br></div><div>Tyler</div><div><br><div><div>On Jul 17, 2014, at 6:51 PM, Alp Toker <<a href="mailto:alp@nuanti.com">alp@nuanti.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br>On 18/07/2014 03:52, Tyler Nowicki wrote:<br><blockquote type="cite">Hi Alp,<br><br>Thanks for the review. I incorporated some of your suggestions. Please review the patches below.<br></blockquote><br>Thanks Tyler, looks mostly good with a couple of tweaks..<br><br><br><blockquote type="cite"><br><blockquote type="cite"><blockquote type="cite"> void<br> OptimizationRemarkHandler(const llvm::DiagnosticInfoOptimizationRemark &D);<br> void OptimizationRemarkHandler(<br> const llvm::DiagnosticInfoOptimizationRemarkMissed &D);<br> void OptimizationRemarkHandler(<br> const llvm::DiagnosticInfoOptimizationRemarkAnalysis &D);<br>+ void OptimizationWarningHandler(<br>+ const llvm::DiagnosticInfoOptimizationWarning &D);<br></blockquote><br>I don't get this. The frontend maps backend remarks to frontend diagnostics so why the special case?<br></blockquote><br>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.<br></blockquote><br>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.<br><br><blockquote type="cite"><br><br><blockquote type="cite"><blockquote type="cite">- Diags.Report(Loc, DiagID) << AddFlagValue(D.getPassName())<br>- << D.getMsg().str();<br>+ // Flag value not used by all optimization messages.<br>+ if (D.getPassName())<br>+ Diags.Report(Loc, DiagID) << AddFlagValue(D.getPassName())<br>+ << D.getMsg().str();<br>+ else<br>+ Diags.Report(Loc, DiagID) << D.getMsg().str();<br></blockquote><br>Is this change necessary? The existing code had the same functionality without the need for an if/else here AFAICT.<br></blockquote><br>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.<br></blockquote><br>Perhaps just use AddFlagValue(D.getPassName() ? D.getPassName() : "") or create a local variable above to avoid duplicating the line.<br><br><blockquote type="cite"><br><blockquote type="cite">I think the plan was to handle this with user-defined diagnostic mappings rather than what's being done here.<br></blockquote><br>What do you mean? Can you elaborate?<br></blockquote><br>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.<br><br><br><blockquote type="cite">--- test/Driver/optimization-failure.cpp (revision 0)<br>+++ test/Driver/optimization-failure.cpp (working copy)<br></blockquote><br>Not a Driver test. If you put this in 'Misc' alongside the other backend diag tests that'll be fine for now.<br><br><br><blockquote type="cite">@@ -0,0 +1,18 @@<br>+// REQUIRES: clang-driver<br></blockquote><br>The 'clang-driver' predicate is something else, shouldn't be needed here.<br><br><blockquote type="cite">+// RUN: %clang %s -O3 -gline-tables-only -gcolumn-info -emit-llvm -S -o /dev/null 2>&1 | FileCheck %s<br>+<br>+<br>+// Test verifies optimization failures generated by the backend are handled<br>+// correctly by clang. LLVM tests verify all of the failure conditions.<br>+// CHECK: {{.*}}:12:5: warning: loop not vectorized: failed explicitly specified loop vectorization<br></blockquote><br>Let's go ahead and test this using the diagnostic verifier (-cc1 -verify) instead of FileCheck.<br><br>See 'test/Misc/backend-stack-frame-diagnostics.cpp' for an example RUN line and expected-warning{{}} directives.<br><br>LGTM with those changes. Feel free to commit both or pass it by me again, whichever you're comfortable with.<br><br>Cheers<br>Alp.<br><br><br><br><blockquote type="cite">+<br>+void test_switch(int *A, int *B, int Length) {<br>+ #pragma clang loop vectorize(enable) unroll(disable)<br>+ for (int i = 0; i < Length; i++) {<br>+ switch(A[i]) {<br>+ case 0: B[i] = 1; break;<br>+ case 1: B[i] = 2; break;<br>+ default: B[i] = 3;<br>+ }<br>+ }<br>+}<br></blockquote><br><br><br><blockquote type="cite"><br>Tyler<br><br>LLVM:<br><br><br><br><br>Clang:<br><br><br><br><br></blockquote><br>--<span class="Apple-converted-space"> </span><br><a href="http://www.nuanti.com/">http://www.nuanti.com</a><br>the browser experts</div></blockquote></div><br></div></body></html>