[PATCH] D32520: Support __fp16 vectors

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added inline comments.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1042
+    }
+
+    assert(SrcElementTy->isFloatingPointTy() &&
----------------
What happens if the SrcElementTy is float and DstElementTy isn't? Seems like it will hit the assertion below.


================
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)) {
----------------
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?


================
Comment at: lib/Sema/SemaExpr.cpp:8072
+/// Convert E, which is a vector, to a vector that has a different element
+/// type.
+static ExprResult convertVector(Expr *E, QualType ElementType, Sema &S) {
----------------
What about `Convert vector E to a vector with the same number of elements but different element type`?


================
Comment at: lib/Sema/SemaExpr.cpp:11316
+// This helper function promotes a binary operator's operands (which are of a
+// half vector type) to a vector of floats and then truncates the result to
+// a vector of either half or short.
----------------
`which are of a half vector type` -> should be there an assertion below to make sure?


================
Comment at: lib/Sema/SemaExpr.cpp:11329
+  QualType BinOpResTy = RHS.get()->getType();
+  if (isVector(ResultTy, Context.ShortTy))
+    BinOpResTy = S.GetSignedVectorType(BinOpResTy);
----------------
Can you add a comment explaining that a) this conversion from float -> int and b) it's needed in case the original binop is a comparison? 


================
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).
----------------
What about `of half vector and truncating the result` to `of half vector to float and truncating the result back to half`


================
Comment at: lib/Sema/SemaExpr.cpp:11568
   
-  if (CompResultTy.isNull())
+  if (CompResultTy.isNull()) {
+    if (ConvertHalfVec)
----------------
Please add a comment here explaining that this path only happens when it's a compound assignment.


================
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 =
----------------
This same logic is used elsewhere in the patch, perhaps factor it out into a static function?


https://reviews.llvm.org/D32520