[Patch][LoopVectorize]Late evaluation of vectorization requirements

[Hal Finkel]

----- Original Message -----
> From: "Tyler Nowicki" <tnowicki at apple.com>
> To: "Hal Finkel" <hfinkel at anl.gov>, "Commit Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>, "llvm cfe"
> <cfe-commits at cs.uiuc.edu>
> Cc: "Gerolf Hoflehner" <ghoflehner at 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 < tnowicki at 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 < hfinkel at anl.gov > wrote:
> 
> ----- Original Message -----
> 
> 
> From: "Tyler Nowicki" < tnowicki at apple.com >
> To: "Commit Messages and Patches for LLVM" < llvm-commits at cs.uiuc.edu
> >
> Cc: "Gerolf Hoflehner" < ghoflehner at apple.com >, "Hal J. Finkel" <
> hfinkel at 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 < tnowicki at 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