[PATCH] D88154: Initial support for vectorization using Libmvec (GLIBC vector math library).

[Florian Hahn via Phabricator via cfe-commits]

fhahn added inline comments.


================
Comment at: llvm/test/Transforms/LoopVectorize/X86/libm-vector-calls-finite.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -vector-library=LIBMVEC-X86 -inject-tli-mappings -loop-vectorize -S < %s | FileCheck %s
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
----------------
fpetrogalli wrote:
> `-inject-tli-mappings` is not required here, as a pass itself is required by the loop vectorizer.
I guess it still doesn't hurt to be explicit. Also, can you add a line for the new pass manager?


================
Comment at: llvm/test/Transforms/LoopVectorize/X86/libm-vector-calls-finite.ll:9
+define void @exp_f32(float* nocapture %varray) {
+; CHECK-LABEL: @exp_f32(
+; CHECK-NEXT:  entry:
----------------
spatel wrote:
> fpetrogalli wrote:
> > I think you are over-testing here. It is enough to check that inside the vector body there is a call to the vector function you have listed in the mapping. You are not checking for the whole auto-vectorization process here, just the vectorization of the function call. Same for all the tests for this patch in which you are doing something similar to this one test.
> > 
> > ```
> > ; CHECK-LABEL: @exp_f32(
> > ; CHECK-LABEL:       vector.body:
> > ; CHECK: call fast <4 x float> @_ZGVbN4v___expf_finite(<4 x float> 
> > ```
> > 
> I requested using "utils/update_test_checks.py" to auto-generate the assertions consistently.
> 
> We have standardized on this practice for tests in several passes because it provides extra test coverage (at the risk of over-testing), and it makes updating tests in the future nearly automatic.
> 
> The time cost of checking the extra lines is negligible vs. the benefit that we have gotten in finding/avoiding bugs. If the consensus is that it's not worth it on this particular file, I'm ok with that. But the general trend is definitely towards auto-generating full checks.
FWIW in this case I would also slightly prefer to have more targeted test lines than auto-generating them (same for most loop-vectorize tests). The tests are large and LV generates a lot of code, and a lot of the code is completely unrelated/uninteresting to the code in the patch. IMO it would be enough to check the arguments of the vector function calls, together with the calls and maybe the induction variable.

The auto-generated check lines make things much more brittle and unrelated changes lead to us requiring to update lots of tests. And I am not sure if it is feasible to audit all details of the generated check lines (in the current patch ~500-800 new CHECK lines). So to me it seems like auto-generating checks here gives a false sense of security and make things harder down the line.


================
Comment at: llvm/test/Transforms/LoopVectorize/X86/libm-vector-calls-finite.ll:82
+
+!1 = distinct !{!1, !2, !3}
+!2 = !{!"llvm.loop.vectorize.width", i32 4}
----------------
I don't think we need this. You can just pass `-force-vector-width=4` to the command line and avoid the extra metadata duplicated for each test


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88154/new/

https://reviews.llvm.org/D88154