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
