This is an archive of the discontinued LLVM Phabricator instance.

[Matrix] Implement C-style explicit type conversions in CXX for matrix types
ClosedPublic

Authored by SaurabhJha on May 1 2021, 1:15 PM.

Details

Summary

This patch implements C-style explicit type conversions in CXX for matrix types. It is part of fixing https://bugs.llvm.org/show_bug.cgi?id=47141

Diff Detail

Event Timeline

SaurabhJha created this revision.May 1 2021, 1:15 PM
SaurabhJha requested review of this revision.May 1 2021, 1:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 1 2021, 1:15 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
fhahn requested changes to this revision.May 2 2021, 5:55 AM

Thanks for the patch!

It looks like there might be a few cases where the wrong conversion is used at the moment.

clang/test/CodeGenCXX/matrix-casts.cpp
27

Shouldn't this use zext for the conversion? Possibly that's an issue with the existing conversion code for matrixes?

67

Shouldn't this use sitofp for the conversion? Possibly that's an issue with the existing conversion code for matrixes?

clang/test/SemaCXX/matrix-casts.cpp
25–30

can you also add a test with an assignment without cast? This should be an error.

This revision now requires changes to proceed.May 2 2021, 5:55 AM
SaurabhJha added inline comments.May 2 2021, 6:09 AM
clang/test/CodeGenCXX/matrix-casts.cpp
27

Would definitely debug it but would like to quickly clarify this.

This is the code that's being executed. For int->int conversion, we are just checking the whether input is signed and regardless of whether output type is signed, we do CreateIntCast. We probably need to check OutputSigned too, right?

fhahn added inline comments.May 2 2021, 6:23 AM
clang/test/CodeGenCXX/matrix-casts.cpp
27

Nevermind, the scalar version I checked used zext due to some AArch64 specific optimization! So sext should be fine here. But that still leaves the int->float case. https://clang.godbolt.org/z/a5Tjed7sP

Updating D101696: [Matrix] Implement C-style explicit type conversions in CXX for matrix types

  1. Remove bitcast when types are of the same size.
  2. Make int conversion depend on whether both input and output are signed.
  3. Add a test case of assigning to an incorrect type.

Thanks for your review @fhahn . I have made some changes in CGExprScalar.cpp to address your comments on IR. I have also added a statement to test assignment to incorrect type.

SaurabhJha updated this revision to Diff 342262.May 2 2021, 1:03 PM

Remove unnecessary newline

SaurabhJha added inline comments.May 2 2021, 1:05 PM
clang/test/CodeGenCXX/matrix-casts.cpp
27

Whoops, did not see your updated comment. Reverting that change.

SaurabhJha updated this revision to Diff 342266.May 2 2021, 1:23 PM

Revert change of sext -> zext

All comments addressed now :)

fhahn added a comment.May 3 2021, 2:11 AM

Thanks for the update! Technically the fix in clang/lib/CodeGen/CGExprScalar.cpp is unrelated to C++ support. It would be great if you could put up a separate patch, so we can land this independently.

The whole patch basically LGTM. It would be great if you could add a test casting from unsigned to float/double (looks like no test generates uitofp) and a test for casting between signed & unsigned integers of the same size (unless there are already tests for that I missed)

clang/lib/CodeGen/CGExprScalar.cpp
1209

Could you s

clang/test/CodeGenCXX/matrix-casts.cpp
5

nit: drop the newline here and before using matrix_5_5...

Thanks for the update! Technically the fix in clang/lib/CodeGen/CGExprScalar.cpp is unrelated to C++ support. It would be great if you could put up a separate patch, so we can land this independently.

The whole patch basically LGTM. It would be great if you could add a test casting from unsigned to float/double (looks like no test generates uitofp) and a test for casting between signed & unsigned integers of the same size (unless there are already tests for that I missed)

Sure thing. I have created a new patch here https://reviews.llvm.org/D101754. Once that's merged, I will rebase this one with the new main.

SaurabhJha updated this revision to Diff 342413.May 3 2021, 9:02 AM

Rebase with main and add new tests for uitofp, unsigned int to int, and int to unsigned int

fhahn accepted this revision.May 3 2021, 12:39 PM

LGTM, thanks!

I can land this on your behalf tomorrow.

After that I think you could apply for commit access https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access :)

This revision is now accepted and ready to land.May 3 2021, 12:39 PM

LGTM, thanks!

I can land this on your behalf tomorrow.

After that I think you could apply for commit access https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access :)

That sounds great, will do!

Perhaps for bonus points, update the Clang documentation in https://clang.llvm.org/docs/LanguageExtensions.html#matrix-types with some examples?

Perhaps for bonus points, update the Clang documentation in https://clang.llvm.org/docs/LanguageExtensions.html#matrix-types with some examples?

Can you please point me to how to submit patches to the documentation? I could only find links like https://clang.llvm.org/hacking.html which talk about code patches. Many thanks.

Perhaps for bonus points, update the Clang documentation in https://clang.llvm.org/docs/LanguageExtensions.html#matrix-types with some examples?

Can you please point me to how to submit patches to the documentation? I could only find links like https://clang.llvm.org/hacking.html which talk about code patches. Many thanks.

It works exactly the same as other patches. I think the file you're interested in is llvm-project/clang/docs/LanguageExtensions.rst.