[PATCH] D32520: Support __fp16 vectors

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:997
 
-  // Allow bitcast from vector to integer/fp of the same size.
-  if (isa<llvm::VectorType>(SrcTy) ||
-      isa<llvm::VectorType>(DstTy))
-    return Builder.CreateBitCast(Src, DstTy, "conv");
+  if (isa<llvm::VectorType>(SrcTy) || isa<llvm::VectorType>(DstTy)) {
+    auto GetSize = [](const llvm::Type *Ty) {
----------------
bruno wrote:
> Please also add a comment explaining what's going on here, like we see for other snippets of logic above.
> 
> It also sounds like this is more generic than it should (which can have unexpected side effects due to the lack of testcases covering vector with other element sizes). I suggest you either (a) add testcases for other sizes or (b) make the condition more restrictive to be sure you're only changing the logic you intend to (i.e., half and i16).
> 
> After these changes, if it makes sense, can you refactor the logic under this condition into its own function? Seems like this function is too big already. 
I'm surprised that we don't have many code-gen tests for vectors, but I can add more test cases that are the same as the tests in fp16vec-ops.c except that the element types are different.

Just to be clear, the only change I'm making here is to handle cases in which the source and destination have different sizes. That shouldn't happen when for example an i8 vector is converted to an i32 because if it did happen, that would have previously caused an assertion when calling CreateBitCast (bitcast requires the sizes of the destination and source be the same).

Also, I removed the lambda and instead used Type::getPrimitiveSizeInBit to compute the vector size.

If this looks OK, I'll try refactoring this function.


================
Comment at: lib/Sema/SemaExpr.cpp:11433
   ExprObjectKind OK = OK_Ordinary;
+  bool ConvertHalfVec = false;
 
----------------
bruno wrote:
> Assuming we're able to handle other vector types here, is it in `ConvertHalfVec` really necessary? It seems odd to me that we need to special case it in every case below. 
ConvertHalfVec is needed to distinguish operations that involve vectors of half from those that don't. If the operands are vectors of half, convertHalfVecBinOp is called to promote the operands and truncate the result.


https://reviews.llvm.org/D32520