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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to