This is an archive of the discontinued LLVM Phabricator instance.

[Matrix] Add documentation for compound assignment and type conversion of matrix types
ClosedPublic

Authored by SaurabhJha on Jun 13 2021, 9:45 AM.

Details

Summary

This patch adds examples of compound assignment and type conversions for matrix types.

Diff Detail

Event Timeline

SaurabhJha created this revision.Jun 13 2021, 9:45 AM
SaurabhJha requested review of this revision.Jun 13 2021, 9:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2021, 9:45 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Forgot to add a colon in code-block header. Fixed.

fhahn added a comment.Jun 14 2021, 7:36 AM

Thanks for the patch! some comments inline.

clang/docs/LanguageExtensions.rst
527

it might also be helpful to include an example with matrix / scalar operations? Division is also supported for matrix/scalar combinations (but not for matrix/matrix)

533

perhaps return the result, so it is not a no-op?

547

can we instead return the casted value, so the code is not a no-op?

Address review comments

Document matrix scalar division

fhahn added inline comments.Jun 17 2021, 1:48 AM
clang/docs/LanguageExtensions.rst
535

Is there any reason we use variables for the scalars? If not, it might be good to keep the examples as compact as possible. Using the extra variables seems overly verbose, when it could be just return (a + 23) * 12 and may give the impression only scalar variables are supported.

same also applies to the other examples below.

578

this could also just be return (fx5x5) i;, right?

Address comment: replace scalar variables by values

Did a --amend to rebuild

Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2021, 7:45 AM

Removing mistakenly added files

fhahn added a comment.Jun 24 2021, 5:43 AM

Thanks for the latest changes!

clang/docs/LanguageExtensions.rst
526

I'm not a native speaker, but I am not sure if between is the best choice here. To me, on would sound slightly better, but again, I'm no expert.

533

nit: only indent 2 spaces, like the others.

562

Maybe just say that explicit conversions are supported, without going into the specific types of casts?

Somehow the builds are failing even though this patch contains no code changes.

clang/docs/LanguageExtensions.rst
526

I think you are right. Will change.

fhahn added a comment.Jun 24 2021, 6:02 AM

Somehow the builds are failing even though this patch contains no code changes.

There a few reasons as to why the tests may fail unrelated to the patch (flaky tests, infrastructure issues, test broken on main), so it is a good idea to check what is failing and why. In this case, it looks like a flaky test.

Address round 2 comments

Rebase with latest main

fhahn accepted this revision.Jun 24 2021, 6:49 AM

LGTM,thanks.

clang/docs/LanguageExtensions.rst
546

nit: also change to on instead of between for consistency?

This revision is now accepted and ready to land.Jun 24 2021, 6:49 AM

Thanks, the build is also passing now so I will land this in a bit.

Sorry, I committed this without the Differential Revision: https://reviews.llvm.org/D104198 line. Is there a way to change the commit message after it is in main? I could not push after git commit --amend

fhahn added a comment.Jun 24 2021, 7:59 AM

Sorry, I committed this without the Differential Revision: https://reviews.llvm.org/D104198 line. Is there a way to change the commit message after it is in main? I could not push after git commit --amend

There's no way to adjust an already pushed commit (force pushes are blocked). You could either revert the change and re-commit with the updated wording or you could manually close the revision here. If the rest of the commit message is OK, I'd recommend just closing the revision manually and provide a link to the commit.

Sorry, I committed this without the Differential Revision: https://reviews.llvm.org/D104198 line. Is there a way to change the commit message after it is in main? I could not push after git commit --amend

There's no way to adjust an already pushed commit (force pushes are blocked). You could either revert the change and re-commit with the updated wording or you could manually close the revision here. If the rest of the commit message is OK, I'd recommend just closing the revision manually and provide a link to the commit.

Okay thanks, I will do the latter.

xgupta added a subscriber: xgupta.Jun 26 2021, 1:33 AM
xgupta added inline comments.
clang/docs/LanguageExtensions.rst
539

This is incorrect ig.

I get warning message :

Warning, treated as error:
/home/user/llvm-project/clang/docs/LanguageExtensions.rst:538:Error in "code-block" directive:
maximum 1 argument(s) allowed, 6 supplied.

xgupta added inline comments.Jun 26 2021, 1:35 AM
clang/docs/LanguageExtensions.rst
538

oh just an blank line is needed :)

SaurabhJha added inline comments.Jun 26 2021, 1:37 AM
clang/docs/LanguageExtensions.rst
538

Thanks, fixing this now.

@fhahn addressed your broadcast comment. Would you prefer that I create the initialisation implementation patch before we get this in?

Sorry, commented on incorrect patch.