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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
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:
----------------
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.


================
Comment at: llvm/test/Transforms/LoopVectorize/X86/libm-vector-calls-finite.ll:69
+  %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %for.body ]
+  %tmp = trunc i64 %indvars.iv to i32
+  %conv = sitofp i32 %tmp to float
----------------
The script should have warned you about using variables named "tmp". Independent of whether we choose to use the scripted assertions or not, you should change this value name (even plain "t" for "trunc" is an improvement over "tmp").


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

https://reviews.llvm.org/D88154