PowerPC] Emit warning for incompatible vector types that are currently diagnosed with -fno-lax-vector-conversions

[maryammo]

Summary

This patch is the last prerequisite to switch the default behaviour to -fno-lax-vector-conversions in the future.
The first path ;D124093; fixed the altivec implicit castings.

[maryammo]

NFC update

[amyk]

clang/include/clang/Basic/DiagnosticSemaKinds.td

7594

clang/lib/Sema/SemaOverload.cpp

1664

Just wanted to check, but is this case covered in the comment above? If it is not, can we add a comment for it?

13052

Unnecessary change?

[maryammo]

Tune the logic to emit warning when at least one of the vectors is altivec one and overloading resolution is done

[amyk]

Some additional minor comments.

clang/include/clang/Basic/DiagnosticSemaKinds.td

7594

nit: Update to

The default behaviour will change to what is implied by the

clang/lib/Sema/SemaExpr.cpp

7736

Can we add some brief documentation for this function, like what is done for other functions in this file?

7737

Can we add a message to this assert()?

7743

nit: Can we put braces on the same line? (And same goes for 7727)

7756

Can we add some brief documentation for this function, like what is done for other functions in this file?

7757

Can we add a message to this assert?

7783

Unnecessary change?

clang/lib/Sema/SemaOverload.cpp

13052

Unnecessary change?

[lei]

Please document all new functions added.

clang/lib/Sema/SemaExpr.cpp

7736

feels like this should be written to just take either 1 param or multiple params via vararg.. since the 2 arg are not really related in any way.

9586

clang-format

10547

clang-format

[maryammo]

clang/lib/Sema/SemaExpr.cpp

7736

sure

7737

I followed what was done for other similar functions like : areMatrixTypesOfTheSameDimension

7737

same as others

7743

sure

7756

sure

7757

I followed what was done for other similar functions like : areMatrixTypesOfTheSameDimension

clang/lib/Sema/SemaOverload.cpp

13052

yes

[maryammo]

clang/lib/Sema/SemaExpr.cpp

7736

I am not sure what you mean, can you please elaborate on that?

[maryammo]

Address the review comments

[maryammo]

NFC

[maryammo]

NFC

[maryammo]

clang-format

[maryammo]

Fixing unintentional merge from previous commit

[amyk]

Thanks for the updates. Unless @lei has any other additional comments, I think LGTM.

[lei]

LGTM, just a few nit that can be addressed on commit.

clang/lib/Sema/SemaExpr.cpp

7736

actually ignore that comment. I see why you need this now.

7744

do we really need this check since we have an assert above?

7747

I don't think the initialization above is needed. Can just inline it here.

bool  isSrcTyAltivec = (SrcVecKind == VectorType::AltiVecVector);

7749

same.

7752

same.

[maryammo]

clang/lib/Sema/SemaExpr.cpp

7744

Yes, without this check there is gonna be a compile time failure for cases where SrcTy is not vector but we wanna cast it to vector 'SrcTy->castAs<VectorType>()'

[maryammo]

clang/lib/Sema/SemaExpr.cpp

7744

the assert checks of either of them is a vector but then we wanna check if at least one of them is altivec.

[lei]

clang/lib/Sema/SemaExpr.cpp

7737

maybe rename to anyAltivecTypes()?

7759

rename suggestion: haveSameVectorElemTypes()

[lei]

clang/lib/Sema/SemaExpr.cpp

7759

rename suggestion: haveSameVectorElemTypes()

actually maybe this: areSameVectorElemTypes()

[nemanjai]

clang/include/clang/Basic/DiagnosticSemaKinds.td

7593–7595

We should not mention the word bitcast in the message as it doesn't really mean anything to C/C++ developers not used to LLVM IR.
We can make the text something like:

Implicit conversion between vector types ('%0' and '%1') is deprecated. In the future, the behavior implied by '-fno-lax-vector-conversions' will be the default.

clang/lib/Sema/SemaExpr.cpp

7739

The message does not need to mention the function name. Something like
"expected at least one type to be a vector here" should suffice.

7741

Nit: naming convention. Here and elsewhere - please review all of your variable names.

7744–7748

I think the whole thing can simply be:

bool IsSrcTyAltivec = SrcTy->isVectorType() &&
  (SrcTy->castAs<VectorType>()->getVectorKind() == VectorType::AltiVecVector);

And similarly for the destination type.

7761

Similarly to my comment above regarding the assert message.

9585

Please add a comment:

// The default for lax vector conversions with Altivec vectors will
// change, so if we are converting between vector types where
// at least one is an Altivec vector, emit a warning.

[maryammo]

Address review comments

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_deprecated_volatile_structured_binding : Warning<
   InGroup<DeprecatedVolatile>;
 
 def warn_deprecated_altivec_src_compat : Warning<
   "Current handling of vector bool and vector pixel types in this context are "
   "deprecated. The default behaviour will soon change to that implied by the "
   "'-altivec-compat=xl' option">,
   InGroup<DiagGroup<"deprecated-altivec-src-compat">>;
 
+def warn_deprecated_lax_vec_conv_all : Warning<
+  "Implicit conversion between vector types ('%0' and '%1') is deprecated. "
+  "In the future, the behavior implied by '-fno-lax-vector-conversions' "
+  "will be the default.">,
+  InGroup<DiagGroup<"deprecate-lax-vec-conv-all">>;
+
 def err_catch_incomplete_ptr : Error<
   "cannot catch pointer to incomplete type %0">;
 def err_catch_incomplete_ref : Error<
   "cannot catch reference to incomplete type %0">;
 def err_catch_incomplete : Error<"cannot catch incomplete type %0">;
 def err_catch_sizeless : Error<
   "cannot catch %select{|reference to }0sizeless type %1">;
 def err_catch_rvalue_ref : Error<"cannot catch exceptions by rvalue reference">;

clang/include/clang/Sema/Sema.h

public:
 
   bool isValidSveBitcast(QualType srcType, QualType destType);
 
   bool areMatrixTypesOfTheSameDimension(QualType srcTy, QualType destTy);
 
   bool areVectorTypesSameSize(QualType srcType, QualType destType);
   bool areLaxCompatibleVectorTypes(QualType srcType, QualType destType);
   bool isLaxVectorConversion(QualType srcType, QualType destType);
+  bool areSameVectorElemTypes(QualType srcType, QualType destType);
+  bool anyAltivecTypes(QualType srcType, QualType destType);
 
   /// type checking declaration initializers (C99 6.7.8)
   bool CheckForConstantInitializer(Expr *e, QualType t);
 
   // type checking C++ declaration initializers (C++ [dcl.init]).
 
   /// ReferenceCompareResult - Expresses the result of comparing two
   /// types (cv1 T1 and cv2 T2) to determine their compatibility for the

clang/lib/Sema/SemaExpr.cpp

bool Sema::areVectorTypesSameSize(QualType SrcTy, QualType DestTy) {
   // power of 2, so instead of using that, we need to use the raw
   // element size multiplied by the element count.
   uint64_t SrcEltSize = Context.getTypeSize(SrcEltTy);
   uint64_t DestEltSize = Context.getTypeSize(DestEltTy);
 
   return (SrcLen * SrcEltSize == DestLen * DestEltSize);
 }
 
+// This returns true if at least one of the types is an altivec vector.
+bool Sema::anyAltivecTypes(QualType SrcTy, QualType DestTy) {
+  assert((DestTy->isVectorType() || SrcTy->isVectorType()) &&
+         "expected at least one type to be a vector here");
+
+  bool IsSrcTyAltivec =
+      SrcTy->isVectorType() && (SrcTy->castAs<VectorType>()->getVectorKind() ==
+                                VectorType::AltiVecVector);
+  bool IsDestTyAltivec = DestTy->isVectorType() &&
+                         (DestTy->castAs<VectorType>()->getVectorKind() ==
+                          VectorType::AltiVecVector);
+
+  return (IsSrcTyAltivec || IsDestTyAltivec);
+}
+
+// This returns true if both vectors have the same element type.
+bool Sema::areSameVectorElemTypes(QualType SrcTy, QualType DestTy) {
+  assert((DestTy->isVectorType() || SrcTy->isVectorType()) &&
+         "expected at least one type to be a vector here");
+
+  uint64_t SrcLen, DestLen;
+  QualType SrcEltTy, DestEltTy;
+  if (!breakDownVectorType(SrcTy, SrcLen, SrcEltTy))
+    return false;
+  if (!breakDownVectorType(DestTy, DestLen, DestEltTy))
+    return false;
+
+  return (SrcEltTy == DestEltTy);
+}
+
 /// Are the two types lax-compatible vector types?  That is, given
 /// that one of them is a vector, do they have equal storage sizes,
 /// where the storage size is the number of elements times the element
 /// size?
 ///
 /// This will also return false if either of the types is neither a
 /// vector nor a real type.
 bool Sema::areLaxCompatibleVectorTypes(QualType srcTy, QualType destTy) {
if (!destTy->isIntegralOrEnumerationType()) {
         return false;
     }
     // OK, integer (vector) -> integer (vector) bitcast.
     break;
 
     case LangOptions::LaxVectorConversionKind::All:
     break;
   }
 
   return areLaxCompatibleVectorTypes(srcTy, destTy);
 }
 
 bool Sema::CheckMatrixCast(SourceRange R, QualType DestTy, QualType SrcTy,
                            CastKind &Kind) {
   if (SrcTy->isMatrixType() && DestTy->isMatrixType()) {
     if (!areMatrixTypesOfTheSameDimension(SrcTy, DestTy)) {
       return Diag(R.getBegin(), diag::err_invalid_conversion_between_matrixes)
if (LHSType->isVectorType() && RHSType->isVectorType()) {
         Kind = CK_BitCast;
         return Compatible;
       }
 
       // If we are allowing lax vector conversions, and LHS and RHS are both
       // vectors, the total size only needs to be the same. This is a bitcast;
       // no bits are changed but the result type is different.
       if (isLaxVectorConversion(RHSType, LHSType)) {
+        // The default for lax vector conversions with Altivec vectors will
+        // change, so if we are converting between vector types where
+        // at least one is an Altivec vector, emit a warning.
+        if (anyAltivecTypes(RHSType, LHSType) &&
+            !areSameVectorElemTypes(RHSType, LHSType))
+          Diag(RHS.get()->getExprLoc(), diag::warn_deprecated_lax_vec_conv_all)
+              << RHSType << LHSType;
         Kind = CK_BitCast;
         return IncompatibleVectors;
       }
     }
 
     // When the RHS comes from another lax conversion (e.g. binops between
     // scalars and vectors) the result is canonicalized as a vector. When the
     // LHS is also a vector, the lax is allowed by the condition above. Handle
     // the case where LHS is a scalar.
     if (LHSType->isScalarType()) {
       const VectorType *VecType = RHSType->getAs<VectorType>();
       if (VecType && VecType->getNumElements() == 1 &&
           isLaxVectorConversion(RHSType, LHSType)) {
+        if (VecType->getVectorKind() == VectorType::AltiVecVector)
+          Diag(RHS.get()->getExprLoc(), diag::warn_deprecated_lax_vec_conv_all)
+              << RHSType << LHSType;
         ExprResult *VecExpr = &RHS;
         *VecExpr = ImpCastExprToType(VecExpr->get(), LHSType, CK_BitCast);
         Kind = CK_BitCast;
         return Compatible;
       }
     }
 
     // Allow assignments between fixed-length and sizeless SVE vectors.
QualType Sema::CheckVectorOperands(ExprResult &LHS, ExprResult &RHS,
   // FIXME: The code below also handles conversion between vectors and
   // non-scalars, we should break this down into fine grained specific checks
   // and emit proper diagnostics.
   QualType VecType = LHSVecType ? LHSType : RHSType;
   const VectorType *VT = LHSVecType ? LHSVecType : RHSVecType;
   QualType OtherType = LHSVecType ? RHSType : LHSType;
   ExprResult *OtherExpr = LHSVecType ? &RHS : &LHS;
   if (isLaxVectorConversion(OtherType, VecType)) {
+    if (anyAltivecTypes(RHSType, LHSType) &&
+        !areSameVectorElemTypes(RHSType, LHSType))
+      Diag(Loc, diag::warn_deprecated_lax_vec_conv_all) << RHSType << LHSType;
     // If we're allowing lax vector conversions, only the total (data) size
     // needs to be the same. For non compound assignment, if one of the types is
     // scalar, the result is always the vector type.
     if (!IsCompAssign) {
       *OtherExpr = ImpCastExprToType(OtherExpr->get(), VecType, CK_BitCast);
       return VecType;
     // In a compound assignment, lhs += rhs, 'lhs' is a lvalue src, forbidding
     // any implicit cast. Here, the 'rhs' should be implicit casted to 'lhs'

clang/lib/Sema/SemaOverload.cpp

bool Sema::IsFunctionConversion(QualType FromType, QualType ToType,
   return true;
 }
 
 /// Determine whether the conversion from FromType to ToType is a valid
 /// vector conversion.
 ///
 /// \param ICK Will be set to the vector conversion kind, if this is a vector
 /// conversion.
-static bool IsVectorConversion(Sema &S, QualType FromType,
-                               QualType ToType, ImplicitConversionKind &ICK) {
+static bool IsVectorConversion(Sema &S, QualType FromType, QualType ToType,
+                               ImplicitConversionKind &ICK, Expr *From,
+                               bool InOverloadResolution) {
   // We need at least one of these types to be a vector type to have a vector
   // conversion.
   if (!ToType->isVectorType() && !FromType->isVectorType())
     return false;
 
   // Identical types require no conversions.
   if (S.Context.hasSameUnqualifiedType(FromType, ToType))
     return false;
static bool IsVectorConversion(Sema &S, QualType FromType, QualType ToType,
   //   same size
   // 3)the destination type does not have the ARM MVE strict-polymorphism
   //   attribute, which inhibits lax vector conversion for overload resolution
   //   only
   if (ToType->isVectorType() && FromType->isVectorType()) {
     if (S.Context.areCompatibleVectorTypes(FromType, ToType) ||
         (S.isLaxVectorConversion(FromType, ToType) &&
          !ToType->hasAttr(attr::ArmMveStrictPolymorphism))) {
+      if (S.isLaxVectorConversion(FromType, ToType) &&
+          S.anyAltivecTypes(FromType, ToType) &&
+          !S.areSameVectorElemTypes(FromType, ToType) &&
+          !InOverloadResolution) {
+        S.Diag(From->getBeginLoc(), diag::warn_deprecated_lax_vec_conv_all)
+            << FromType << ToType;
+      }
       ICK = ICK_Vector_Conversion;
       return true;
     }
   }
 
   return false;
 }
 
if (S.Context.hasSameUnqualifiedType(FromType, ToType)) {
     // Pointer conversions (C++ 4.10).
     SCS.Second = ICK_Pointer_Conversion;
     SCS.IncompatibleObjC = IncompatibleObjC;
     FromType = FromType.getUnqualifiedType();
   } else if (S.IsMemberPointerConversion(From, FromType, ToType,
                                          InOverloadResolution, FromType)) {
     // Pointer to member conversions (4.11).
     SCS.Second = ICK_Pointer_Member;
-  } else if (IsVectorConversion(S, FromType, ToType, SecondICK)) {
+  } else if (IsVectorConversion(S, FromType, ToType, SecondICK, From,
+                                InOverloadResolution)) {
     SCS.Second = SecondICK;
     FromType = ToType.getUnqualifiedType();
   } else if (!S.getLangOpts().CPlusPlus &&
              S.Context.typesAreCompatible(ToType, FromType)) {
     // Compatible conversions (Clang extension for C function overloading)
     SCS.Second = ICK_Compatible_Conversion;
     FromType = ToType.getUnqualifiedType();
   } else if (IsTransparentUnionStandardConversion(S, From, ToType,
else
     NewFn = SemaRef.BuildDeclarationNameExpr(SS, R, false);
 
   if (NewFn.isInvalid())
     return ExprError();
 
   // This shouldn't cause an infinite loop because we're giving it
   // an expression with viable lookup results, which should never
   // end up here.
   return SemaRef.BuildCallExpr(/*Scope*/ nullptr, NewFn.get(), LParenLoc,
                                MultiExprArg(Args.data(), Args.size()),
                                RParenLoc);
 }
 
 /// Constructs and populates an OverloadedCandidateSet from
 /// the given function.
 /// \returns true when an the ExprResult output parameter has been set.
 bool Sema::buildOverloadedCallSet(Scope *S, Expr *Fn,

clang/test/Parser/cxx-altivec.cpp

-// RUN: %clang_cc1 -triple=powerpc-apple-darwin8 -target-feature +altivec -fsyntax-only -verify=expected,novsx -std=c++11 %s
-// RUN: %clang_cc1 -triple=powerpc64-unknown-linux-gnu -target-feature +altivec -target-feature +vsx -fsyntax-only -verify=expected,nonaix -std=c++11 %s
-// RUN: %clang_cc1 -triple=powerpc64le-unknown-linux-gnu -target-feature +altivec -fsyntax-only -verify=expected,novsx -std=c++11 %s
-// RUN: %clang_cc1 -triple=powerpc64-unknown-linux-gnu -target-feature +vsx -target-cpu pwr7 -fsyntax-only -verify=expected,nonaix -std=c++11 %s
-// RUN: %clang_cc1 -triple=powerpc64le-unknown-linux-gnu -target-feature -vsx -target-cpu pwr7 -fsyntax-only -verify=expected,novsx -std=c++11 %s
-// RUN: %clang_cc1 -triple=powerpc-ibm-aix -target-feature +altivec -fsyntax-only -verify=expected,aix -std=c++11 %s
-// RUN: %clang_cc1 -triple=powerpc64-ibm-aix -target-feature +altivec -fsyntax-only -verify=expected,aix -std=c++11 %s
+// RUN: %clang_cc1 -triple=powerpc-apple-darwin8 -Wno-deprecate-lax-vec-conv-all -target-feature +altivec -fsyntax-only -verify=expected,novsx -std=c++11 %s
+// RUN: %clang_cc1 -triple=powerpc64-unknown-linux-gnu -Wno-deprecate-lax-vec-conv-all -target-feature +altivec -target-feature +vsx -fsyntax-only -verify=expected,nonaix -std=c++11 %s
+// RUN: %clang_cc1 -triple=powerpc64le-unknown-linux-gnu -Wno-deprecate-lax-vec-conv-all -target-feature +altivec -fsyntax-only -verify=expected,novsx -std=c++11 %s
+// RUN: %clang_cc1 -triple=powerpc64-unknown-linux-gnu -Wno-deprecate-lax-vec-conv-all -target-feature +vsx -target-cpu pwr7 -fsyntax-only -verify=expected,nonaix -std=c++11 %s
+// RUN: %clang_cc1 -triple=powerpc64le-unknown-linux-gnu -Wno-deprecate-lax-vec-conv-all -target-feature -vsx -target-cpu pwr7 -fsyntax-only -verify=expected,novsx -std=c++11 %s
+// RUN: %clang_cc1 -triple=powerpc-ibm-aix -Wno-deprecate-lax-vec-conv-all -target-feature +altivec -fsyntax-only -verify=expected,aix -std=c++11 %s
+// RUN: %clang_cc1 -triple=powerpc64-ibm-aix -Wno-deprecate-lax-vec-conv-all -target-feature +altivec -fsyntax-only -verify=expected,aix -std=c++11 %s
 #include <altivec.h>
 
 __vector char vv_c;
 __vector signed char vv_sc;
 __vector unsigned char vv_uc;
 __vector short vv_s;
 __vector signed  short vv_ss;
 __vector unsigned  short vv_us;