This patch includes the LoopVectorize changes necessary to call simd functions and is associated with revision D22792.
Details
Diff Detail
Event Timeline
lib/Analysis/LoopAccessAnalysis.cpp | ||
---|---|---|
1735 | I don't think that this is obvious. If IsAnnotatedParallel is true, then this is fine. If not, I don't think that vector-variants implies anything about the memory-referencing behavior of the variant (and it might be some external thing we can't analyze). I imagine that, at least most of the time, when locally defined, these will be argmemonly functions (but I don't think that even that is guaranteed, unfortunately). Also, if we've been told to vectorize, and assured that it is safe, we should do this even if there are calls without vector variants (we'll just scalarize in that case). I see no reason that a function with inapplicable vector variants should be special in this sense. You might want to do that and then rebase on top of that change. | |
lib/Analysis/VectorUtils.cpp | ||
601 | Please make this comment more formal. In part, you should explain that the ABI for _Complex is platform dependent (including at the IR level). | |
lib/Transforms/Vectorize/LoopVectorize.cpp | ||
4731 | the only -> The only | |
4736 | the only -> The only | |
4965 | SimdVariant might be nullptr; don't segfault in the DEBUG statement in that case. | |
5082 | Don't do this. VecClone needs to be able to detect when it doesn't need to generate a new function. |
Hi Matt, this is very nice. I have a couple of comments to add to Hal's one.
- I would like to see some description of which bits of the patch the tests are verifying. For example, I can see that you have two similar tests with masked and unmasked functions, but it is not clear from the IR I see and from the opt invocation why one tests generates a masked call and the other one an unmasked one. It would be great if you could also add the original C code you intended to vectorize with this patch as a comment.
- Would it be possible to reduce the tests to minimal size? For example, you have generated "vector-variants"="_ZGVbN8vlu_dowork,_ZGVcN8vlu_dowork,_ZGVdN8vlu_dowork,_ZGVeN8vlu_dowork,_ZGVbM8vlu_dowork,_ZGVcM8vlu_dowork,_ZGVdM8vlu_dowork,_ZGVeM8vlu_dowork", but it should also work with vector-variants consisting only of the single variant you want to use in the vecttorizer.
- You are testing linear and uniform. Could you also add tests for simple cases like the following?
#pragma omp declare simd linear(y) double f(double x) { //... } void loop(double *x, double *y, int N) { for (int i = 0; i < N; ++i) { y[i] = f(x[i]); } }
include/llvm/Analysis/VectorUtils.h | ||
---|---|---|
182 | Given that this is likely to change for different architectures, I wonder whether it is worth to redirect it to an overload method of TargetTransformInfo. | |
test/Transforms/LoopVectorize/masked_simd_func.ll | ||
91 | Could you unit test this only adding the _ZGVdM8vlu_dowork variant to the attribute? |
Given that this is likely to change for different architectures, I wonder whether it is worth to redirect it to an overload method of TargetTransformInfo.