----- Original Message ----- > From: "Tyler Nowicki" <tnowi...@apple.com> > To: "Hal Finkel" <hfin...@anl.gov>, "Commit Messages and Patches for LLVM" > <llvm-comm...@cs.uiuc.edu>, "llvm cfe" > <cfe-commits@cs.uiuc.edu> > Cc: "Gerolf Hoflehner" <ghofleh...@apple.com> > Sent: Monday, July 20, 2015 1:40:28 PM > Subject: Re: [Patch][LoopVectorize]Late evaluation of vectorization > requirements > > Hi, > > > Could I get a review of these patches for cfe and llvm?
Hi Tyler, I'm apologize for the delay. I think this generally looks good, but I don't understand the motivation for introducing the additional FrontendOptions member. Why not just make a subclass of DiagnosticInfoOptimizationRemarkAnalysis that the frontend can handle specially (and detect using the normal isa/dyn_cast mechanism? > > > I should have also said in my previous email that I am not thrilled > by the need to use O3 in the clang-side test. I need the vectorizer > to generate the backend diagnostic message so the front-end can > append the appropriate compiler options. The clang-side test > verifies that the correct compiler options are being appended to the > backend diagnostic message that informs the user of the fp commute > issue. > > > I have a llvm-side test to verify the backend diagnostic message is > generated correctly. I need the clang-side test to verify the > front-end recognizes the message and appends the correct compiler > options. The inliner is tested in a similar way in the front-end > without O3 because it is triggered with cc1 without any command line > options. Unfortunately the command line options described in ' > http://llvm.org/docs/Vectorizers.html' don’t seem to trigger the > vectorizer without O3. So using -O2 or using -fvectorize does not help? -Hal > > > Thanks, > > > Tyler > > > > > > On Jul 15, 2015, at 3:55 PM, Tyler Nowicki < tnowi...@apple.com > > wrote: > > Hi Hal, > > Thanks of the review! I split the message moving the front-end > specific parts into clang. Rather than using a special subclass for > this message I used the existing subclass > DiagnosticOptimizationRemarkAnalysis and added a parameter for the > category of front-end options that should be appended to the > message. In the future I will extend this to include several other > categories of analysis messages. > > I didn’t put the category parameter higher in the subclass hierarchy > (for example in DiagnosticOptimizationRemark) because I think it > only makes sense for analysis remarks to be followed by a list of > front-end specific options. > > [Clang Patch] > - Select diagnostic message based on the options category. > > [LLVM Patch] > - Specified options category when generating diagnostic message. > > Tyler > > <Clang-Add-diagnostic-to-append-fp-commute-clang-options.patch> > <LLVM-Late-evaluation-of-vectorization-requirements.patch> > > > > > On Jul 9, 2015, at 7:09 PM, Hal Finkel < hfin...@anl.gov > wrote: > > ----- Original Message ----- > > > From: "Tyler Nowicki" < tnowi...@apple.com > > To: "Commit Messages and Patches for LLVM" < llvm-comm...@cs.uiuc.edu > > > Cc: "Gerolf Hoflehner" < ghofleh...@apple.com >, "Hal J. Finkel" < > hfin...@anl.gov > > Sent: Sunday, June 28, 2015 3:17:01 PM > Subject: Re: [Patch][LoopVectorize]Late evaluation of vectorization > requirements > > > > Improved patch with clang-format. > > > Tyler > > > On Jun 28, 2015, at 1:09 PM, Tyler Nowicki < tnowi...@apple.com > > wrote: > > > Hi, > > > > To vectorize fp reductions fast-math is required, but this check is > done while identifying the reduction causing a very cryptical > analysis message. Also it should be possible to provide a loop hint > rather than specifying fast-math for the whole module. > > > > For float reductions the current analysis message reads: > - value that could not be identified as reduction is used outside the > loop > > > This patch moves the verification of fast-math, adds a check for a > loop hint, and improves the analysis message. > > > The improved message reads: > - vectorization requires changes in the order of operations, however > IEEE 754 floating-point operations are not commutative; allow > commutativity by specifying ‘#pragma clang loop vectorize(enable)’ > before the loop or by providing the compiler option ‘-ffast-math’ > > > + "commutative; allow commutativity by specifying ‘#pragma clang " > + "loop vectorize(enable)’ before the loop or by providing the " > + "compiler option ‘-ffast-math’"; > + } > + llvm_unreachable("Unknown requirement error"); > > I agree that this is the best kind of message, but Clang is not the > only LLVM frontend, and we should not talk about Clang options here. > When we designed the diagnostic system, we specifically gave it a > class hierarchy so that the frontend could treat some messages > specially, and this seems like a perfect example of where we should > do so (by having a special subclass for these kinds of vectorizer > messages, and having Clang do special things with them). > > -Hal > > > > > > In the future I will extend this to include pointer aliasing checks > and perhaps cost-model checks. > > > This patch builds off of my other diagnostic changes that are > currently in review ( http://reviews.llvm.org/D10714 ) and “Renaming > and Diagnostics for Loop Interleaving". Please make sure to apply > patches 0002 and 0003 if you want to try this patch. > > > Comments are much appreciated! > > > Tyler > > > <0004-Late-evaluation-of-vectorization-requirements.patch> > > > > > -- > Hal Finkel > Assistant Computational Scientist > Leadership Computing Facility > Argonne National Laboratory > > > -- 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