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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
