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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
