-
Notifications
You must be signed in to change notification settings - Fork 184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: extend intrinsic matmul
#951
Open
wassup05
wants to merge
11
commits into
fortran-lang:master
Choose a base branch
from
wassup05:matmul
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
f06f556
add interface and procedures
wassup05 fed4d73
add implementation for 3,4,5 matrices
wassup05 27911ae
add very basic example
wassup05 a7f645c
fix typo
wassup05 cc77dee
a bit efficient
wassup05 3958018
refactor algorithm
wassup05 35a5a28
add new interface
wassup05 ebf92d7
add helper functions
wassup05 5f5c5a9
add implementation, refactor select to if clauses
wassup05 06ce735
slightly better examples
wassup05 e709f83
replace all matmul's by gemm
wassup05 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev
Previous commit
replace all matmul's by gemm
- Loading branch information
commit e709f838aeb1b57276bdfe36840ef9a720e1086c
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good progress @wassup05, thank you! Imho this PR is almost ready to be merged. As you suggest, it would be good to have a nice wrapper for
gemm
. It has been discussed before. I would like to suggest that all calls togemm
are also wrapped into astdlib_matmul
function - now with two matrices only. This would give stdlib fully functional matmul functionality.Here I suggest two possible APIs, and I will ask @jalvesz @jvdp1 @loiseaujc to discuss that together:
The first would be similar to
gemm
and could use the matrix state definitions already in use for the sparse operations
stdlib/src/stdlib_sparse_constants.fypp
Lines 16 to 18 in 5c64ee6
The second would be more ambitious and essentially zero-overhead, it would wrap the operation in a derived type: (to be templated of course)
Then we could define a templated base interface
So the user writing code would have it clear:
we could even make it an operator:
without it triggering any actual data movement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would that work in the written code? Would
A
andB
have to be declared astype(matrix_state_type)
?If so, I'm a bit afraid most people would prefer the more "natural"
real, dimension(m, n) :: A, B
and play around either directly with the intrinsictranspose
or the classicalgemm
dummy arguments. Maybe I'm missing something though?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also had the same reaction at first, after thinking about it longer I saw that it is actually a pretty clever solution, the user would declare the matrices as regular dense matrices. It is the internal interface that would make the distinction. This would imply though implementing internally several versions to account for the combinations (dense,dense) / (dense,type) / (type,dense). This looks interesting but I wonder if it should be pursued at this stage. The first proposal by @perazz looks easier and totally valid but I would propose it with a slight modification:
to let all optional arguments of a procedure at the end of the signature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes @jalvesz @loiseaujc, I don't know how to write it better, but it is outlined in the previous post.
There would be an interface for
matmul(A,B)
(resolved at compile time) with 4 options for each kind:matrix_state_
, B is a 2D arraymatrix_state_
, A is a 2d arraymatrix_state_
*Only function 4. is actually implemented, and is a
gemm
wrapper: the other implementations just wrap against it withfypp
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see. I quite like that indeed. I still believe though that, as a starting point, restricting ourselves to standard stuff might be easier. Introducing new derived types to represent matrices definitely is something I'm looking forward to but, in line with the discussion here, it might require a broader discussion to have a well-designed set of derived types.
I also prefer this signature
for the reasons you've mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, here I would define these "convenience" derived types, because they're only used to make the
matmul
interface better and more readable. I would only expose to the user the actual interface (i.e., either the operators.t.
.h.
, or the function namestransposed
hermitian
).