Fix an error after D21678

[vbyakovlcl]

Summary

This fixes an error and gcc incompatibilities in vector shifts reported by Akira Hatanaka

---

From: Akira Hatanaka <[e-mail]>
Date: Fri, Sep 2, 2016 at 3:00 AM
Subject: Re: [PATCH] D21678: Fix For pr28288 - Error message in shift of vector values
To: [e-mail], [e-mail], [e-mail], [e-mail], [e-mail]
Cc: [e-mail], [e-mail], [e-mail], [e-mail], [e-mail]

ahatanak added a subscriber: ahatanak.
ahatanak added a comment.

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?

Repository:

rL LLVM

https://reviews.llvm.org/D21678

[ahatanak]

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

8733

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?

8746

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.

8751

This comment should be updated since other vector types (e.g., gcc's vector type) are handled by this function too.

8773

Is it correct to always create an ExtVectorType here? What if the RHS is a GenericVector?

8786

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?

8790

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?

[vbyakovlcl]

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

8733

I cannot remember why I did this change. Removing it doesn't cause any new fails.

8746

I added a comment.

8786

Ok. I removed this and will fail a new review after committing of this review.

8790

No need, because CodeGen inserts a conversion.

define <8 x i16> @foo1(<8 x i16> %n, i32 %m) #0 {
entry:

%n.addr = alloca <8 x i16>, align 16
%m.addr = alloca i32, align 4
store <8 x i16> %n, <8 x i16>* %n.addr, align 16
store i32 %m, i32* %m.addr, align 4
%0 = load <8 x i16>, <8 x i16>* %n.addr, align 16
%1 = load i32, i32* %m.addr, align 4
%splat.splatinsert = insertelement <8 x i32> undef, i32 %1, i32 0
%splat.splat = shufflevector <8 x i32> %splat.splatinsert, <8 x i32> undef, <8 x i32> zeroinitializer
%sh_prom = trunc <8 x i32> %splat.splat to <8 x i16>
%shl = shl <8 x i16> %0, %sh_prom
ret <8 x i16> %shl

We need insert a conversion of scalar LHS to the RHS element type because we must return a vector of the RHS type.

[ahatanak]

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

8751

Should we mention that any vectors used as shift operands have to have integer element types? This comment gives the impression that only OpenCL vectors need to be integers.

8790

OK. So the rule is that the type of the scalar operand is normally converted to the vector element type of the other operand before being transformed to a vector, but we don't have to do this when it's used as the RHS (shift amount) because IRGen generates the correct IR without the conversion?

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

56

I don't think this patch was necessary to have clang print diagnostic "vector operands do not have the same number of elements"? If that's the case, I think it's better to add these lines in a separate commit.

[vbyakovlcl]

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

8790

Yes. I think a CodeGen test should be added for checking of IR correctness.

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

56

Do you propose to delete lines with this diagnostic?

[ahatanak]

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

56

Yes, I think you can move them to a separate patch. Doing so will help people who read the patch understand what problems you are trying to fix with this patch.

[ahatanak]

LGTM with a nit in test case.

llvm/tools/clang/test/CodeGen/vecshift.c

43 ↗

(On Diff #71498)

This test fails on my machine because MaxVectorAlign is 128 (16B) for darwin. Maybe you can remove the alignment checks or add a triple to the RUN line?

[vbyakovlcl]

llvm/tools/clang/test/CodeGen/vecshift.c

43 ↗

(On Diff #71498)

Done

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

S.Diag(Loc, diag::warn_shift_result_gt_typewidth)
     << RHS.get()->getSourceRange();
 }
 
 /// \brief Return the resulting type when a vector is shifted
 ///        by a scalar or vector shift amount.
 static QualType checkVectorShift(Sema &S, ExprResult &LHS, ExprResult &RHS,
                                  SourceLocation Loc, bool IsCompAssign) {
   // OpenCL v1.1 s6.3.j says RHS can be a vector only if LHS is a vector.
-  if (!LHS.get()->getType()->isVectorType()) {
+  if ((S.LangOpts.OpenCL || S.LangOpts.ZVector) &&
+      !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();
   }
 
   if (!IsCompAssign) {
     LHS = S.UsualUnaryConversions(LHS.get());
     if (LHS.isInvalid()) return QualType();
   }
 
   RHS = S.UsualUnaryConversions(RHS.get());
   if (RHS.isInvalid()) return QualType();
 
   QualType LHSType = LHS.get()->getType();
-  const VectorType *LHSVecTy = LHSType->castAs<VectorType>();
-  QualType LHSEleType = LHSVecTy->getElementType();
+  // Note that LHS might be a scalar because the routine calls not only in
+  // OpenCL case.
+  const VectorType *LHSVecTy = LHSType->getAs<VectorType>();
+  QualType LHSEleType = LHSVecTy ? LHSVecTy->getElementType() : LHSType;
 
   // Note that RHS might not be a vector.
   QualType RHSType = RHS.get()->getType();
   const VectorType *RHSVecTy = RHSType->getAs<VectorType>();
   QualType RHSEleType = RHSVecTy ? RHSVecTy->getElementType() : RHSType;
 
   // OpenCL v1.1 s6.3.j says that the operands need to be integers.
   if (!LHSEleType->isIntegerType()) {
     S.Diag(Loc, diag::err_typecheck_expect_int)
       << LHS.get()->getType() << LHS.get()->getSourceRange();
     return QualType();
   }
 
   if (!RHSEleType->isIntegerType()) {
     S.Diag(Loc, diag::err_typecheck_expect_int)
       << RHS.get()->getType() << RHS.get()->getSourceRange();
     return QualType();
   }
 
-  if (RHSVecTy) {
+  if (!LHSVecTy) {
+    assert(RHSVecTy);
+    if (IsCompAssign)
+      return RHSType;
+    if (LHSEleType != RHSEleType) {
+      LHS = S.ImpCastExprToType(LHS.get(),RHSEleType, CK_IntegralCast);
+      LHSEleType = RHSEleType;
+    }
+    QualType VecTy =
+        S.Context.getExtVectorType(LHSEleType, RHSVecTy->getNumElements());
+    LHS = S.ImpCastExprToType(LHS.get(), VecTy, CK_VectorSplat);
+    LHSType = VecTy;
+  } else 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...
     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 VecTy =
       S.Context.getExtVectorType(RHSEleType, LHSVecTy->getNumElements());
     RHS = S.ImpCastExprToType(RHS.get(), VecTy, CK_VectorSplat);
   }
 
   return LHSType;
 }
 
 // C99 6.5.7
 QualType Sema::CheckShiftOperands(ExprResult &LHS, ExprResult &RHS,
                                   SourceLocation Loc, BinaryOperatorKind Opc,

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

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+typedef __attribute__((__ext_vector_type__(8))) char vector_char8;
+typedef __attribute__((__ext_vector_type__(8))) short vector_short8;
+typedef __attribute__((__ext_vector_type__(8))) int vector_int8;
+typedef __attribute__((__ext_vector_type__(8))) unsigned char vector_uchar8;
+typedef __attribute__((__ext_vector_type__(8))) unsigned short vector_ushort8;
+typedef __attribute__((__ext_vector_type__(8))) unsigned int vector_uint8;
+typedef __attribute__((__ext_vector_type__(4))) char vector_char4;
+typedef __attribute__((__ext_vector_type__(4))) short vector_short4;
+typedef __attribute__((__ext_vector_type__(4))) int vector_int4;
+typedef __attribute__((__ext_vector_type__(4))) unsigned char vector_uchar4;
+typedef __attribute__((__ext_vector_type__(4))) unsigned short vector_ushort4;
+typedef __attribute__((__ext_vector_type__(4))) unsigned int vector_uint4;
+
+char c;
+short s;
+int i;
+unsigned char uc;
+unsigned short us;
+unsigned int ui;
+vector_char8 vc8;
+vector_short8 vs8;
+vector_int8 vi8;
+vector_uchar8 vuc8;
+vector_ushort8 vus8;
+vector_uint8 vui8;
+vector_char4 vc4;
+vector_short4 vs4;
+vector_int4 vi4;
+vector_uchar4 vuc4;
+vector_ushort4 vus4;
+vector_uint4 vui4;
+
+void foo() {
+  vc8 = 1 << vc8;
+  vuc8 = 1 << vuc8;
+  vi8 = 1 << vi8;
+  vui8 = 1 << vui8;
+  vs8 = 1 << vs8;
+  vus8 = 1 << vus8;
+
+  vc8 = c << vc8;
+  vuc8 = i << vuc8;
+  vi8 = uc << vi8;
+  vui8 = us << vui8;
+  vs8 = ui << vs8;
+  vus8 = 1 << vus8;
+
+  vc8 = vc8 << vc8;
+  vi8 = vi8 << vuc8;
+  vuc8 = vuc8 << vi8;
+  vus8 = vus8 << vui8;
+  vui8 = vui8 << vs8;
+
+  vc4 = vc4 << vc8; // expected-error {{vector operands do not have the same number of elements}}
+  vi4 = vi4 << vuc8; // expected-error {{vector operands do not have the same number of elements}}
+  vuc4 = vuc4 << vi8; // expected-error {{vector operands do not have the same number of elements}}
+  vus4 = vus4 << vui8; // expected-error {{vector operands do not have the same number of elements}}
+  vui4 = vui4 << vs8; // expected-error {{vector operands do not have the same number of elements}}
+
+
+  vc8 = vc8 << vc4; // expected-error {{vector operands do not have the same number of elements}}
+  vi8 = vi8 << vuc4; // expected-error {{vector operands do not have the same number of elements}}
+  vuc8 = vuc8 << vi4; // expected-error {{vector operands do not have the same number of elements}}
+  vus8 = vus8 << vui4; // expected-error {{vector operands do not have the same number of elements}}
+  vui8 = vui8 << vs4; // expected-error {{vector operands do not have the same number of elements}}
+
+  vc8 <<= vc8;
+  vi8 <<= vuc8;
+  vuc8 <<= vi8;
+  vus8 <<= vui8;
+  vui8 <<= vs8;
+
+  c <<= vc8; // expected-error {{assigning to 'char' from incompatible type}}
+  i <<= vuc8; // expected-error {{assigning to 'int' from incompatible type}}
+  uc <<= vi8; // expected-error {{assigning to 'unsigned char' from incompatible type}}
+  us <<= vui8; // expected-error {{assigning to 'unsigned short' from incompatible type}}
+  ui <<= vs8; // expected-error {{assigning to 'unsigned int' from incompatible type}}
+}