Fix For pr28288 - Error message in shift of vector values

[• vbyakovl]

Summary

This fixes an error in type checking of shift of vector values.

[Anastasia]

llvm/tools/clang/lib/Sema/SemaExpr.cpp

8595

an vector -> a vector

8674–8675

it seems like you don't need this statement as the next one is exactly the same!

[• vbyakovl]

Ping!

[• vbyakovl]

Someone, please review this.

[aaron.ballman]

llvm/tools/clang/lib/Sema/SemaExpr.cpp

8597

Why does this drop the mention of OpenCL in the function name?

8638

It's good to keep language references in the comments, so I would leave the reference in even though this is being expanded for non-OpenCL vector types (unless the reference is incorrect).

8681–8683

Why is it correct to remove semantic checking for vector operands?

llvm/tools/clang/test/Sema/shift.c

75

Please clang-format the test case.

[• vbyakovl]

llvm/tools/clang/lib/Sema/SemaExpr.cpp

8597

This routine has common applying rather than for OpenCL only.

8638

I'll restore the language reference.

8681–8683

This routine targets for checking operands of operations like plus, mul ..., but not shifts. The tests vect_shift_1 and vect_shift_2 (I added) are examples which were compiled with error.

llvm/tools/clang/test/Sema/shift.c

75

Ok.

[aaron.ballman]

llvm/tools/clang/lib/Sema/SemaExpr.cpp

8681–8683

Are you saying that calling CheckVectorOperands was always incorrect? Or that it's no longer required because it should be fully covered by checkVectorShift? Because the two methods do considerably different checking, and I would have expected there to be more behavioral differences in the tests by removing the call to CheckVectorOperands that suggests there are tests missing.

[• vbyakovl]

llvm/tools/clang/lib/Sema/SemaExpr.cpp

8681–8683

Yes, calling CheckVectorOperands is not correct here because it compares operand types to be the same. For shift it is not true.

[aaron.ballman]

LGTM, but I am also not well-versed in vector operations, so you may want to wait a day or two for @uweigand or someone else to sign off as well.

llvm/tools/clang/lib/Sema/SemaExpr.cpp

8681–8683

Thank you for clarifying!

[andreybokhanko]

@uweigand Ulrich, any objections here? If not, I will commit this patch tomorrow (as the author, Vladimir, doesn't have commit access yet).

Andrey

[ahatanak]

This patch causes clang to error out on the following code, which used to compile fine:

$ cat f2.c

typedef __attribute__((__ext_vector_type__(8))) unsigned short vector_ushort8;

vector_ushort8 foo1(void) {
  return 1 << (vector_ushort8){7,6,5,4,3,2,1,0};
}

$ clang f2.c -c

clang used to transform the scaler operand to a vector operand (similar to the way gcc's vector is handled) when compiling for normal c/c++ (but printed an error message when compiling for opencl), but this patch dropped the check for LangOpts added in r230464 and changed that behavior. I don't think this was intentional?

llvm/tools/clang/lib/Sema/SemaExpr.cpp

static void DiagnoseBadShiftValues(Sema& S, ExprResult &LHS, ExprResult &RHS,
   }
 
   S.Diag(Loc, diag::warn_shift_result_gt_typewidth)
     << HexResult.str() << Result.getMinSignedBits() << LHSType
     << Left.getBitWidth() << LHS.get()->getSourceRange()
     << RHS.get()->getSourceRange();
 }
 
-/// \brief Return the resulting type when an OpenCL vector is shifted
+/// \brief Return the resulting type when a vector is shifted
 ///        by a scalar or vector shift amount.
-static QualType checkOpenCLVectorShift(Sema &S,
-                                       ExprResult &LHS, ExprResult &RHS,
-                                       SourceLocation Loc, bool IsCompAssign) {
+static QualType checkVectorShift(Sema &S, ExprResult &LHS, ExprResult &RHS,
   // OpenCL v1.1 s6.3.j says RHS can be a vector only if LHS is a vector.
   if (!LHS.get()->getType()->isVectorType()) {
     S.Diag(Loc, diag::err_shift_rhs_only_vector)
       << RHS.get()->getType() << LHS.get()->getType()
       << LHS.get()->getSourceRange() << RHS.get()->getSourceRange();
     return QualType();
   }
 
static QualType checkVectorShift(Sema &S, ExprResult &LHS, ExprResult &RHS,
 
   if (!RHSEleType->isIntegerType()) {
     S.Diag(Loc, diag::err_typecheck_expect_int)
       << RHS.get()->getType() << RHS.get()->getSourceRange();
     return QualType();
   }
 
   if (RHSVecTy) {
-    // OpenCL v1.1 s6.3.j says that for vector types, the operators
-    // are applied component-wise. So if RHS is a vector, then ensure
-    // that the number of elements is the same as LHS...
+    // For vector types, the operators are applied component-wise. So if RHS is
+    // a vector, then ensure that the number of elements is the same as LHS...
     if (RHSVecTy->getNumElements() != LHSVecTy->getNumElements()) {
       S.Diag(Loc, diag::err_typecheck_vector_lengths_not_equal)
         << LHS.get()->getType() << RHS.get()->getType()
         << LHS.get()->getSourceRange() << RHS.get()->getSourceRange();
       return QualType();
     }
   } else {
     // ...else expand RHS to match the number of elements in LHS.

 QualType Sema::CheckShiftOperands(ExprResult &LHS, ExprResult &RHS,
                                   SourceLocation Loc, BinaryOperatorKind Opc,
                                   bool IsCompAssign) {
   checkArithmeticNull(*this, LHS, RHS, Loc, /*isCompare=*/false);
 
   // Vector shifts promote their scalar inputs to vector type.
   if (LHS.get()->getType()->isVectorType() ||
       RHS.get()->getType()->isVectorType()) {
-    if (LangOpts.OpenCL)
-      return checkOpenCLVectorShift(*this, LHS, RHS, Loc, IsCompAssign);
     if (LangOpts.ZVector) {
       // The shift operators for the z vector extensions work basically
-      // like OpenCL shifts, except that neither the LHS nor the RHS is
+      // like general shifts, except that neither the LHS nor the RHS is
       // allowed to be a "vector bool".
       if (auto LHSVecType = LHS.get()->getType()->getAs<VectorType>())
         if (LHSVecType->getVectorKind() == VectorType::AltiVecBool)
           return InvalidOperands(Loc, LHS, RHS);
       if (auto RHSVecType = RHS.get()->getType()->getAs<VectorType>())
         if (RHSVecType->getVectorKind() == VectorType::AltiVecBool)
           return InvalidOperands(Loc, LHS, RHS);
-      return checkOpenCLVectorShift(*this, LHS, RHS, Loc, IsCompAssign);
     }
-    return CheckVectorOperands(LHS, RHS, Loc, IsCompAssign,
-                               /*AllowBothBool*/true,
-                               /*AllowBoolConversions*/false);
+    return checkVectorShift(*this, LHS, RHS, Loc, IsCompAssign);
   }
 
   // Shifts don't perform usual arithmetic conversions, they just do integer
   // promotions on each operand. C99 6.5.7p3
 
   // For the LHS, do usual unary conversions, but then reset them away
   // if this is a compound assignment.
   ExprResult OldLHS = LHS;

llvm/tools/clang/test/Sema/shift.c

void test_pr5544() {
   (void) (((1) > 63 && (1) < 128 ? (((unsigned long long) 1)<<((1)-64)) : (unsigned long long) 0)); // no-warning
 }
 
 void test_shift_too_much(char x) {
   if (0)
     (void) (x >> 80); // no-warning
   (void) (x >> 80); // expected-warning {{shift count >= width of type}}
 }
+
+typedef unsigned vec16 __attribute__ ((vector_size (16)));
+typedef unsigned vec8 __attribute__ ((vector_size (8)));
+
+void
+vect_shift_1 (vec16 *x)
+{
+   *x = *x << 4 ;
+}
+
+void
+vect_shift_2 (vec16 *x,vec16 y)
+{
+   *x = *x << y ;
+}
+
+void
+vect_shift_3 (vec16 *x,vec8 y)
+{
+   *x = *x << y ; // expected-error {{vector operands do not have the same number of elements}}
+}