[PATCH] D40575: LoopVectorize support for simd functions

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli added a comment.

Hi Matt, this is very nice. I have a couple of comments to add to Hal's one.

1. 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.
2. 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.
3. 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]);
    }
  } 



================
Comment at: include/llvm/Analysis/VectorUtils.h:182
+/// according to the vector function ABI.
+Type* calcCharacteristicType(Function& F, VectorVariant& Variant);
+
----------------
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.


================
Comment at: test/Transforms/LoopVectorize/masked_simd_func.ll:91
+
+attributes #0 = { noinline nounwind uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="core-avx2" "target-features"="+aes,+avx,+avx2,+bmi,+bmi2,+cx16,+f16c,+fma,+fsgsbase,+fxsr,+lzcnt,+mmx,+movbe,+pclmul,+popcnt,+rdrnd,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave,+xsaveopt" "unsafe-fp-math"="false" "use-soft-float"="false" "vector-variants"="_ZGVbN4vlu_dowork,_ZGVcN8vlu_dowork,_ZGVdN8vlu_dowork,_ZGVeN16vlu_dowork,_ZGVbM4vlu_dowork,_ZGVcM8vlu_dowork,_ZGVdM8vlu_dowork,_ZGVeM16vlu_dowork" }
+attributes #1 = { noinline nounwind uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="core-avx2" "target-features"="+aes,+avx,+avx2,+bmi,+bmi2,+cx16,+f16c,+fma,+fsgsbase,+fxsr,+lzcnt,+mmx,+movbe,+pclmul,+popcnt,+rdrnd,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave,+xsaveopt" "unsafe-fp-math"="false" "use-soft-float"="false" }
----------------
Could you unit test this only adding the _ZGVdM8vlu_dowork variant to the attribute? 


https://reviews.llvm.org/D40575