Thanks! Committed in r243315.
Tyler > On Jul 26, 2015, at 4:27 AM, Hal Finkel <hfin...@anl.gov> wrote: > > ----- Original Message ----- >> From: "Tyler Nowicki" <tnowi...@apple.com <mailto:tnowi...@apple.com>> >> To: "Hal Finkel" <hfin...@anl.gov <mailto:hfin...@anl.gov>> >> Cc: "llvm cfe" <cfe-commits@cs.uiuc.edu <mailto:cfe-commits@cs.uiuc.edu>>, >> "Gerolf Hoflehner" <ghofleh...@apple.com <mailto:ghofleh...@apple.com>> >> Sent: Monday, July 20, 2015 1:06:28 PM >> Subject: Re: [Patch][LoopInfo] Use CGLoopInfo to generate loop hints >> >> Hi, >> >> Could I get a review of the latest patch? > > This LGTM, thanks! > > -Hal > >> >> Thanks, >> >> >> Tyler >> >> >> On Jul 14, 2015, at 4:45 PM, Tyler Nowicki < tnowi...@apple.com > >> wrote: >> >> Hi Hal, >> >> >> Thanks for the review! I have committed the renaming patch. >> >> >> Comments in-line and I’ve attached an updated patch for review >> >> >> >> >> >> >> On Jul 9, 2015, at 6:12 PM, Hal Finkel < hfin...@anl.gov > wrote: >> >> Hi Tyler, >> >> 0001-Rename-Vectorizer-to-Vectorize-and-VectorizeUnroll-t.patch: >> >> This makes sense; it makes the variable names match the name of the >> metadata they control. LGTM. >> >> 0002-Moved-functionality-from-EmitCondBr-to-CGLoopInfo.patch: >> >> + ValueInt = static_cast<unsigned>(ValueAPS.getSExtValue()); >> >> >> >> Removed. >> >> >> >> >> >> Do Clang or GCC warn here without the static cast? If not, remove it. >> >> + case LoopHintAttr::UnrollCount: >> + case LoopHintAttr::VectorizeWidth: >> + case LoopHintAttr::InterleaveCount: >> + break; >> >> The fact that there is no code here for either LoopHintAttr::Disable >> or case LoopHintAttr::Enable seems counter-intuitive. If this is >> right, please add a comment explaining why. >> >> >> >> Specifying something like vectorizea_width(enable/disable) is invalid >> and produces an error. Perhaps we could improve on the message: >> error: use of undeclared identifier ‘disable’. So the case >> statements should never be triggered. I’ll add unreachable() calls >> to make sure that is clear. >> >> >> >> >> >> --- a/test/CodeGenCXX/pragma-loop.cpp >> +++ b/test/CodeGenCXX/pragma-loop.cpp >> @@ -10,7 +10,7 @@ void while_test(int *List, int Length) { >> #pragma clang loop vectorize_width(4) >> #pragma clang loop unroll(full) >> while (i < Length) { >> - // CHECK: br i1 {{.*}}, label {{.*}}, label {{.*}}, !llvm.loop >> ![[LOOP_1:.*]] >> + // CHECK: br label {{.*}}, !llvm.loop ![[LOOP_1:.*]] >> List[i] = i * 2; >> i++; >> } >> >> Why are these things changing? Are we now attaching the metadata to a >> different branch? >> >> >> >> Yes, that is part of the reason for this change. >> >> >> >> >> >> >> >> >> >> >> >> >> "I noticed a problem with my implementation of #pragma clang loop >> >> >> >> >> >> >> >> >> >> >> vectorize(assume_safety). When it was specified on a loop other >> >> >> >> >> >> >> >> >> >> >> loop hints were lost. The problem is that CGLoopInfo attaches >> >> >> >> >> >> >> >> >> >> >> metadata a little bit differently than EmitCondBrHints in CGStmt. >> >> >> >> >> >> >> >> >> >> >> For do-loops CGLoopInfo attaches metadata to the br in the body >> >> >> >> >> >> >> >> >> >> >> block and for while and for loops, the inc block. EmitCondBrHints >> >> >> >> >> >> >> >> >> >> >> on the other hand always attaches data to the br in the cond >> >> >> >> >> >> >> >> >> >> >> block. When specifying assume_safety CGLoopInfo emits an empty >> >> >> >> >> >> >> >> >> >> >> llvm.loop metadata shadowing the metadata in the cond block. Loop >> >> >> >> >> >> >> >> >> >> >> transformations like rotate and unswitch would then eliminate the >> cond block.” >> >> >> So when vectorize(assume_safety) is specified EmitCondBrHints would >> add the loop hint vectorize.enable = true to the cond br. But that >> would be lost by loop transformations. There certainly are other >> methods of fixing this problem, but I think unifying how metadata is >> attached makes the most sense and is the best for llvm in the long >> term. >> >> >> I will post a patch for documentation of assume_safety once this >> problem has been fixed. >> >> >> Tyler >> >> <0001-Moved-functionality-from-EmitCondBr-to-CGLoopInfo.patch> >> > > -- > Hal Finkel > Assistant Computational Scientist > Leadership Computing Facility > Argonne National Laboratory
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits