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

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli added a comment.

Hi,

Thank you for modifying the implementation to facilitate the extension to other targets.

Please add `libmvec`-specific tests is `clang/Driver/fveclib.c`, and simplify the loop-vectorize tests.

Other than that, I only have minor comments.

Francesco



================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:377
+      default:
+	break;
+      case llvm::Triple::x86_64:
----------------
Nit: is this misaligned?


================
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"
----------------
`-inject-tli-mappings` is not required here, as a pass itself is required by the loop vectorizer.


================
Comment at: llvm/test/Transforms/LoopVectorize/X86/libm-vector-calls-finite.ll:6
+
+declare float @__expf_finite(float) #0
+
----------------
Nit: it is standard practice to put all declarations at the end of the file.


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



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

https://reviews.llvm.org/D88154