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

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7571-7573
+  "Current bitcast for incompatible vector types (%0 and %1) are deprecated. "
+  "The default behaviour will change to what is implied by the "
+  "-fno-lax-vector-conversions option">,
----------------
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.
```


================
Comment at: clang/lib/Sema/SemaExpr.cpp:7718
+  assert((DestTy->isVectorType() || SrcTy->isVectorType()) &&
+         "areAnyVectorTypesAltivec - invalid input types");
+
----------------
The message does not need to mention the function name. Something like
`"expected at least one type to be a vector here"` should suffice.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:7720
+
+  bool isSrcTyAltivec = false;
+  bool isDestTyAltivec = false;
----------------
Nit: naming convention. Here and elsewhere - please review all of your variable names.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:7723-7727
+  if (SrcTy->isVectorType()) {
+    VectorType::VectorKind SrcVecKind =
+        SrcTy->castAs<VectorType>()->getVectorKind();
+    isSrcTyAltivec = (SrcVecKind == VectorType::AltiVecVector);
+  }
----------------
maryammo wrote:
> maryammo wrote:
> > lei wrote:
> > > do we really need this check since we have an assert above?
> > 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>()' 
> the assert checks of either of them is a vector but then we wanna check if at least one of them is altivec. 
I think the whole thing can simply be:
```
bool IsSrcTyAltivec = SrcTy->isVectorType() &&
  (SrcTy->castAs<VectorType>()->getVectorKind() == VectorType::AltiVecVector);
```
And similarly for the destination type.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:7740
+  assert((DestTy->isVectorType() || SrcTy->isVectorType()) &&
+         "areVectorTypesSameElmType -invalid input types");
+
----------------
Similarly to my comment above regarding the assert message.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:9571
       if (isLaxVectorConversion(RHSType, LHSType)) {
+        if (areAnyVectorTypesAltivec(RHSType, LHSType) &&
+            !areVectorTypesSameElmType(RHSType, LHSType))
----------------
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.
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126540/new/

https://reviews.llvm.org/D126540