[PATCH] D24467: Fix an error after D21678

Akira Hatanaka via cfe-commits cfe-commits at lists.llvm.org
Mon Sep 12 15:00:15 PDT 2016


ahatanak added inline comments.

================
Comment at: llvm/tools/clang/lib/Sema/SemaExpr.cpp:8733
@@ -8731,2 +8732,3 @@
   if (!IsCompAssign) {
-    LHS = S.UsualUnaryConversions(LHS.get());
+    if (S.LangOpts.OpenCL || S.LangOpts.ZVector)
+      LHS = S.UsualUnaryConversions(LHS.get());
----------------
It wasn't clear to me why we have to call DefaultFunctionArrayLvalueConversion instead of UsualUnaryConversions. Is it possible to come up with a test case that would fail without this change, and if it is, can you add it to vecshift.c?

================
Comment at: llvm/tools/clang/lib/Sema/SemaExpr.cpp:8747
@@ -8742,3 +8746,3 @@
 
   // Note that RHS might not be a vector.
   QualType RHSType = RHS.get()->getType();
----------------
We no longer error out when LHS is not a vector, so it should mention that either the LHS or the RHS might not be a vector.

================
Comment at: llvm/tools/clang/lib/Sema/SemaExpr.cpp:8752
@@ -8747,3 +8751,3 @@
 
   // OpenCL v1.1 s6.3.j says that the operands need to be integers.
   if (!LHSEleType->isIntegerType()) {
----------------
This comment should be updated since other vector types (e.g., gcc's vector type) are handled by this function too.

================
Comment at: llvm/tools/clang/lib/Sema/SemaExpr.cpp:8774
@@ +8773,3 @@
+    QualType VecTy =
+        S.Context.getExtVectorType(LHSEleType, RHSVecTy->getNumElements());
+    LHS = S.ImpCastExprToType(LHS.get(), VecTy, CK_VectorSplat);
----------------
Is it correct to always create an ExtVectorType here? What if the RHS is a GenericVector?

================
Comment at: llvm/tools/clang/lib/Sema/SemaExpr.cpp:8787
@@ -8770,1 +8786,3 @@
     }
+    if (!S.LangOpts.OpenCL && !S.LangOpts.ZVector) {
+      const BuiltinType *LHSBT = LHSEleType->getAs<clang::BuiltinType>();
----------------
Is it possible to split this out to another patch? The first patch would fix handling of (scalar << vector) and the second patch would make clang reject vector shifts if element sizes of the LHS and RHS are different. It's not clear whether it's correct to reject the latter case, so perhaps you can discuss it in a separate patch? 

================
Comment at: llvm/tools/clang/lib/Sema/SemaExpr.cpp:8847
@@ -8774,3 +8846,3 @@
       S.Context.getExtVectorType(RHSEleType, LHSVecTy->getNumElements());
     RHS = S.ImpCastExprToType(RHS.get(), VecTy, CK_VectorSplat);
   }
----------------
When the LHS is a scalar, we check the type of the LHS and the element type of the RHS, and if necessary, cast the LHS to the element type of the RHS. What's the reason we don't do the same here?


Repository:
  rL LLVM

https://reviews.llvm.org/D24467





More information about the cfe-commits mailing list