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 ↗

(On Diff #62345)

an vector -> a vector

8675 ↗

(On Diff #62345)

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 ↗

(On Diff #62464)

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

8638 ↗

(On Diff #62464)

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 ↗

(On Diff #62464)

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

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

75 ↗

(On Diff #62464)

Please clang-format the test case.

[• vbyakovl]

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

8597 ↗

(On Diff #62464)

This routine has common applying rather than for OpenCL only.

8638 ↗

(On Diff #62464)

I'll restore the language reference.

8681–8683 ↗

(On Diff #62464)

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 ↗

(On Diff #62464)

Ok.

[aaron.ballman]

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

8681–8683 ↗

(On Diff #67140)

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 ↗

(On Diff #67140)

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 ↗

(On Diff #67140)

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?

cfe/trunk/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();
   }
 

 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;

cfe/trunk/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}}
+}