[PATCH] D18701: [ARM] Adding IEEE-754 SIMD detection to loop vectorizer

[Renato Golin via llvm-commits]

rengolin added inline comments.

================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:372
@@ +371,3 @@
+  /// (like sub-normals in NEON v7) via unsafe math flags.
+  bool enablePotentiallyUnsafeFPVectorization() const;
+
----------------
hfinkel wrote:
> The name of this method, and the corresponding comment, seem potentially confusing (no pun intended ;) ). It does not explain which floating-point operations are potentially unsafe (essentially all of them?). I think it would be much better to name this:
> 
>   bool isFPVectorizationPotentiallyUnsafe() const;
> 
> perhaps with this comment:
> 
>   /// \brief Indicate that it is potentially unsafe to automatically vectorize floating-point operations because the semantics of vector and scalar floating-point semantics may differ. For example, ARM NEON v7 SIMD math does not support IEEE-754 denormal numbers, while depending on the platform, scalar floating-point math does.
> 
I see, and then invert the logic to return "false" by default, as in "it's not unsafe to transform this operation in this target", but on v7 NEON return true because it "is unsafe to do unsafe FP on NEON". I'll change that, as I really hated my original name. :)

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4727
@@ +4726,3 @@
+      } else if (it->getType()->isFloatingPointTy() &&
+                (CI || it->isBinaryOp() || it->isCast() || it->isShift()) &&
+                !it->hasUnsafeAlgebra()) {
----------------
hfinkel wrote:
> I don't understand why you have the `(CI || it->isBinaryOp() || it->isCast() || it->isShift())` - why does it matter what kind of operation it is? And do we have any floating-point shifts? If you specifically want to include casts, do you also specifically need to pick up loads and stores?
We don't want to get loads and stores because they're normally safe anyway. That's why I'm using the "else if".

But also I don't want to get MOVs and shuffles either, since there's no chance they'll change the semantics or precision unless it's specifically requested (widening/shortening, type change, etc). So I just selected everything else. :)

Function calls and binary operations are obvious. Casts may change the type, so they may end up as the case above. I don't know of any FP shifts, so that was probably over the top.

I think it should be safe to do:

    } else if (it->getType()->isFloatingPointTy() &&
                 (CI || it->isBinaryOp()) &&
                 !it->hasUnsafeAlgebra()) {



http://reviews.llvm.org/D18701