----- Original Message ----- > From: "Tyler Nowicki" <[email protected]> > To: "Hal Finkel" <[email protected]> > Cc: "llvm cfe" <[email protected]>, "Gerolf Hoflehner" > <[email protected]> > 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 < [email protected] > > 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 < [email protected] > 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 [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
