Page MenuHomePhabricator

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

Authored by maryammo on May 27 2022, 7:05 AM.

Details

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.

Diff Detail

Event Timeline

maryammo created this revision.May 27 2022, 7:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2022, 7:05 AM
maryammo requested review of this revision.May 27 2022, 7:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2022, 7:05 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
maryammo updated this revision to Diff 432553.May 27 2022, 7:20 AM

NFC update

amyk added inline comments.Mon, May 30, 8:58 PM
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 updated this revision to Diff 437161.Wed, Jun 15, 7:28 AM

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

amyk added a comment.Wed, Jun 15, 10:02 AM

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 added a comment.Wed, Jun 15, 10:46 AM

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 added inline comments.Wed, Jun 15, 10:48 AM
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 added inline comments.Wed, Jun 15, 11:03 AM
clang/lib/Sema/SemaExpr.cpp
7736

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

maryammo updated this revision to Diff 437303.Wed, Jun 15, 12:53 PM
maryammo marked an inline comment as not done.

Address the review comments

maryammo updated this revision to Diff 437313.Wed, Jun 15, 1:12 PM

clang-format

maryammo updated this revision to Diff 437317.Wed, Jun 15, 1:27 PM

Fixing unintentional merge from previous commit

amyk accepted this revision.Wed, Jun 15, 4:44 PM

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

This revision is now accepted and ready to land.Wed, Jun 15, 4:44 PM
lei added a comment.Wed, Jun 15, 7:37 PM

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 added inline comments.Wed, Jun 15, 8:12 PM
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 added inline comments.Wed, Jun 15, 8:21 PM
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 added inline comments.Thu, Jun 16, 5:40 AM
clang/lib/Sema/SemaExpr.cpp
7737

maybe rename to anyAltivecTypes()?

7759

rename suggestion: haveSameVectorElemTypes()

lei added inline comments.Thu, Jun 16, 5:44 AM
clang/lib/Sema/SemaExpr.cpp
7759

rename suggestion: haveSameVectorElemTypes()

actually maybe this: areSameVectorElemTypes()

nemanjai added inline comments.Thu, Jun 16, 2:25 PM
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 updated this revision to Diff 437740.Thu, Jun 16, 3:47 PM

Address review comments

This revision was landed with ongoing or failed builds.Thu, Jun 16, 6:28 PM
This revision was automatically updated to reflect the committed changes.