[PATCH] D32520: Support __fp16 vectors

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Hi Akira,

This is nice, thanks for doing it!



================
Comment at: include/clang/Sema/Sema.h:9270
+                                               QualType RHSType,
+                                               bool CompAssign = false);
 
----------------
Can you change `CompAssign` to `IsCompAssign` so that we match the pattern used in other methods?


================
Comment at: include/clang/Sema/Sema.h:9279
+                                               bool ConvertRHS = true,
+                                               bool CompAssign = false);
 
----------------
Same here.


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


================
Comment at: lib/Sema/SemaExpr.cpp:11433
   ExprObjectKind OK = OK_Ordinary;
+  bool ConvertHalfVec = false;
 
----------------
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. 


================
Comment at: test/Sema/fp16vec-sema.c:47
+  // clang currently disallows using these operators on vectors, which is
+  // allowed by gcc.
+  sv0 = !hv0; // expected-error{{invalid argument type}}
----------------
Can you add a FIXME here?


https://reviews.llvm.org/D32520