[PATCH] D32520: Support __fp16 vectors

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1042
+    }
+
+    assert(SrcElementTy->isFloatingPointTy() &&
----------------
bruno wrote:
> What happens if the SrcElementTy is float and DstElementTy isn't? Seems like it will hit the assertion below.
That's not supposed to happen since there are only three cases we have to consider here:

1. truncation from an int vector to a short vector
2. promotion from a half vector to a float vector
3. truncation from a float vector to a half vector

I've cleaned up the assertions and moved them up to make the code clearer.


================
Comment at: lib/Sema/SemaExpr.cpp:7511
+      // If this is a compound assignment, allow converting the RHS to the type
+      // of the LHS.
+      if (IsCompAssign && isVector(LHSType, Context.HalfTy)) {
----------------
bruno wrote:
> Since it seems that you're always doing the same conversion (the only variable input here is the number of elements), can you update the comment to mention the exact conversion?
It turns out this code isn't needed at all. I accidentally left the code in that I had in my local branch. The conversions between half vectors and float vectors take place after the operands are checked, so both the LHS and the RHS are still half vectors at this point.

Sorry for causing confusion.


================
Comment at: lib/Sema/SemaExpr.cpp:11537
+  // Some of the binary operations require promoting operands of half vector
+  // and truncating the result. For now, we do this only when HalfArgsAndReturn
+  // is set (that is, when the target is arm or arm64).
----------------
bruno wrote:
> What about `of half vector and truncating the result` to `of half vector to float and truncating the result back to half`
I also changed the comment in CreateBuiltinUnaryOp.


================
Comment at: lib/Sema/SemaExpr.cpp:11978
+    // truncating the result. For now, we do this only when HalfArgsAndReturns
+    // is set (that is, when the target is arm or arm64).
+    ConvertHalfVec =
----------------
bruno wrote:
> This same logic is used elsewhere in the patch, perhaps factor it out into a static function?
I factored it out to function 'needsConversionOfHalfVec'.


https://reviews.llvm.org/D32520