This is an archive of the discontinued LLVM Phabricator instance.

[Docs] Define matrix initialisation in MatrixTypes documentation
AbandonedPublic

Authored by SaurabhJha on Jul 14 2021, 11:51 AM.

Details

Summary

As part of https://bugs.llvm.org/show_bug.cgi?id=46251, this patch adds definition of matrix initialisation. I am not very familiar with this part of C++
so please let me know if I am wrong here, which I suspect I am.

I have been trying to follow the reference documentation https://en.cppreference.com/w/cpp/language/initialization and the commented out test in the
bugzilla ticket https://github.com/llvm/llvm-project/blob/3323a628ec821b8b75d3b60bf1510931f97d3883/clang/test/CodeGenCXX/matrix-type-builtins.cpp#L78

Diff Detail

Event Timeline

SaurabhJha requested review of this revision.Jul 14 2021, 11:51 AM
SaurabhJha created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2021, 11:51 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
fhahn added inline comments.Jul 20 2021, 2:57 AM
clang/docs/MatrixTypes.rst
271

is there a reason we need to allow 'empty' initialisation? What does it mean?

SaurabhJha added inline comments.Jul 20 2021, 11:15 AM
clang/docs/MatrixTypes.rst
271

By empty, I meant this {} initialisation which was in the test case. I got the term wrong but seems like we do want to support {} initialisation, right?

Updated docs to address comments

rjmccall added inline comments.Jul 21 2021, 2:30 PM
clang/docs/MatrixTypes.rst
279

This is contradicted by your first example. I think you want to say something like

A value of matrix type M can be initialized with an initializer list:

(examples)

If the initializer list is empty, all elements of the matrix are zero-initialized. Otherwise, the initializer list must consist of M::rows initializer lists, each of which must consist of M::columns expressions, each of which is used to initialize the corresponding element of the matrix (that is, m[i][j] is initialized by the jth expression of the ith initializer list in the initializer). Element designators are not allowed.

That's assuming you want to allow {}, but I think that's probably a good idea. At the very least, you already have to define how objects of static storage duration are zero-initialized, and having a way to do that explicitly always seems wise to me.

SaurabhJha added inline comments.Jul 22 2021, 12:28 AM
clang/docs/MatrixTypes.rst
279

That sounds great @rjmccall, thanks for your suggestions.

I think I got confused between whether to allow {} but as you suggested, I think we should.

Address second round of comments

fhahn added a comment.Jul 22 2021, 1:05 AM

Thank you very much for working on this! Are you planning on implementing the new specification as well? It would probably be good to land the update to the spec in close succession to the implementation, to avoid confusing users.

clang/docs/MatrixTypes.rst
279

That looks good, thanks!

Another thing to consider is whether we would like to provide a convenient syntax to initialise all elements with an user-specified value, i.e. should we allow something like m = {99};, which would broadcast 99 to all elements of the matrix?

Thank you very much for working on this! Are you planning on implementing the new specification as well? It would probably be good to land the update to the spec in close succession to the implementation, to avoid confusing users.

Yes, that's my plan. Once this is in, I will start working on the implementation right away.

clang/docs/MatrixTypes.rst
279

That sounds good. Do we have a way to infer the number of rows and columns for a matrix object m so that we could broadcast correctly? I will amend the docs later today.

Add documentation for matrix broadcast initialization

fhahn added a comment.Jul 26 2021, 8:29 AM

Thank you very much for working on this! Are you planning on implementing the new specification as well? It would probably be good to land the update to the spec in close succession to the implementation, to avoid confusing users.

Yes, that's my plan. Once this is in, I will start working on the implementation right away.

Ok cool! I think the latest version looks good (modulo making sure the new lines are limited to 80 chars per line). @rjmccall can you think of any scenarios where defining initializers with one expression and broadcasting them might cause issues?

With respect to ordering the patches, I think it would be good to put up a patch implementing the newly added parts, commit it and then land the patch that adds it to the docs. WDYT?

Thank you very much for working on this! Are you planning on implementing the new specification as well? It would probably be good to land the update to the spec in close succession to the implementation, to avoid confusing users.

Yes, that's my plan. Once this is in, I will start working on the implementation right away.

Ok cool! I think the latest version looks good (modulo making sure the new lines are limited to 80 chars per line). @rjmccall can you think of any scenarios where defining initializers with one expression and broadcasting them might cause issues?

With respect to ordering the patches, I think it would be good to put up a patch implementing the newly added parts, commit it and then land the patch that adds it to the docs. WDYT?

Yeah, sounds good. I will create a patch for implementing initialisation.

fhahn added a comment.Sep 26 2022, 6:20 AM

Thank you very much for working on this! Are you planning on implementing the new specification as well? It would probably be good to land the update to the spec in close succession to the implementation, to avoid confusing users.

Yes, that's my plan. Once this is in, I will start working on the implementation right away.

Ok cool! I think the latest version looks good (modulo making sure the new lines are limited to 80 chars per line). @rjmccall can you think of any scenarios where defining initializers with one expression and broadcasting them might cause issues?

With respect to ordering the patches, I think it would be good to put up a patch implementing the newly added parts, commit it and then land the patch that adds it to the docs. WDYT?

Yeah, sounds good. I will create a patch for implementing initialisation.

@SaurabhJha did you ever get a chance to create a patch?

Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2022, 6:20 AM
SaurabhJha abandoned this revision.Sep 26 2022, 9:46 AM
SaurabhJha added a subscriber: Florian.

@SaurabhJha did you ever get a chance to create a patch?

Hey @Florian, sorry I dropped the ball here. I don't think I would be able to spend time on this so abandoning this revision.