[PATCH] D40575: LoopVectorize support for simd functions

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1735
+            Call->getCalledFunction()->hasFnAttribute("vector-variants"))
+          continue;
+
----------------
I don't think that this is obvious. If IsAnnotatedParallel is true, then this is fine. If not, I don't think that vector-variants implies anything about the memory-referencing behavior of the variant (and it might be some external thing we can't analyze). I imagine that, at least most of the time, when locally defined, these will be argmemonly functions (but I don't think that even that is guaranteed, unfortunately).

Also, if we've been told to vectorize, and assured that it is safe, we should do this even if there are calls without vector variants (we'll just scalarize in that case). I see no reason that a function with inapplicable vector variants should be special in this sense. You might want to do that and then rebase on top of that change.


================
Comment at: lib/Analysis/VectorUtils.cpp:601
+
+  // TODO except Clang's ComplexType
+  if (!CharacteristicDataType || CharacteristicDataType->isStructTy()) {
----------------
Please make this comment more formal. In part, you should explain that the ABI for _Complex is platform dependent (including at the IR level).


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4731
+    //
+    // 1) the only available simd function variants have a VF that is less than
+    //    the loop VF. In this case, multiple calls can be made to the simd
----------------
the only -> The only


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4736
+    //
+    // 2) the only available simd function variants have a VF that is greater
+    //    than the loop VF. In this case, we can make the call to the simd
----------------
the only -> The only


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4965
+      SimdVariant = matchVectorVariant(F, VF, IsMasked, TTI);
+      DEBUG(dbgs() << "Matched Variant: " << SimdVariant->encode() << "\n");
+      SimdFuncParms = SimdVariant->getParameters();
----------------
SimdVariant might be nullptr; don't segfault in the DEBUG statement in that case.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5082
+      // in case VecClone runs again.
+      F->removeFnAttr("vector-variants");
+      delete SimdVariant;
----------------
Don't do this. VecClone needs to be able to detect when it doesn't need to generate a new function.


https://reviews.llvm.org/D40575