Skip to content
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

Generic Matrix kernel specializations #722

Merged
merged 16 commits into from
May 16, 2024
Merged

Conversation

AlexRTer
Copy link
Collaborator

@AlexRTer AlexRTer commented May 3, 2024

This PR adds implementations using the generic Matrix type for most kernels.
They only use methods that are sure to be supported by Matrix.h based classes, i.e. get, set, prepare-/finish-/append.

Hence, new classes based on Matrix.h do not necessarily have to implement specializations for all kernels in order to make use of them. The implementations do not assume any additional constraints (e.g. memory layout) which limits possibilities for optimization in some cases. Therefore, they do not replace specialized kernels entirely but save a great amount of time by providing basic utility "out of the box". Note that this PR only adds the necessary specialized kernels and does not make them accessible to the runtime yet.

Changes:

  • Matrix.h specializations have been added to almost all local kernels with exception of e.g. Neural Network related kernels.

  • Existing tests have been extended to cover these as well. Currently, the kernels always default to a DenseMatrix if res was given as a nullptr, so tests also use a DenseMatrix through GenGivenVals.

    • Since empty matrices and views cannot be obtained this way, some tests use DTEmpty or DTView to generate these before they are cast to the appropriate type.
      • e.g. using DTEmpty = DenseMatrix<typename DTArg::VT>;
        static_cast<DTArg *>(DataObjectFactory::create<DTEmpty>(0, 0, false));
    • Some kernels like Filter* also expect sel to be a DenseMatrix, in which cases DTSel is sometimes used.
    • Some tests have a different DTRes type than DTArg, so DT might be set using std::conditional
  • DenseMatrix.h has also been changed to handle empty matrices if prepare-/finish- Append is called on them. I suspect this might also be an issue with e.g. CSRMatrix but I have not yet confirmed this.

To do:
The cast (l. 327) in Order.h should be fine, though restrictive, as VTRes = VTArg is guaranteed in that case but please confirm this. The problem lies in extractRow expecting res and arg to have the same value type, which is true if returnIdx = false, but the compiler seems to also check if a cast is legal for the case of returnIdx = true in which value types are not guaranteed to be the same - so a static cast does not work.

@pdamme pdamme self-requested a review May 8, 2024 16:28
pdamme added 3 commits May 9, 2024 17:50
- Removed newly added <cassert> include from kernels, since not used there (and we don't want to use assertions anymore).
- Undid unnecessary move of Matrix code in EwBinaryObjSca.cpp.
- Undid the trick about std::accumulate() in FilterCol and FilterRow kernels.
  - The input matrix sel could be a view into a larger matrix, but std::accumulate() does not take the rowSkip of sel into account.
- Fixed indentation and a typo in the newly added code in Write.h.
- Some more minor things.
Copy link
Collaborator

@pdamme pdamme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @AlexRTer, thanks for this contribution. The kernel specializations for the superclass Matrix are very useful as fallbacks whenever the specialization for a particular physical matrix representation is missing. We expect such situations to become more important as we add additional matrix representations.

The code in this PR looks very good to me (even though I mostly skimmed some of the kernels and test cases). I made a few little changes (see my commit).

Regarding your question on the cast in the Order-kernel: The concern is valid as that cast indeed looks a bit fishy. I added a fix for it that avoids the cast by leveraging the if constexpr (...) to make sure that the ExtractRow-kernel is instantiated/called only when VTRes and VTArg are the same. Otherwise, an exception is thrown.

From my point of view, this PR is ready to be merged. But as you created it as a draft, let me know if you have any further comments or if I may merge it in.

@pdamme
Copy link
Collaborator

pdamme commented May 9, 2024

Thanks also for cleaning up the existing kernels and their test cases a bit (e.g., consistent file structure, corrections of misformattings and misnamings, compaction of multiple DataObjectFactory::destroy() into a single one, adding missing destroys() etc.). Very much appreciated!

@AlexRTer AlexRTer marked this pull request as ready for review May 10, 2024 12:16
@AlexRTer
Copy link
Collaborator Author

Thank you for looking at the cast in the Order.h kernel.
You can go ahead and merge the PR.

@pdamme pdamme merged commit 2b4f086 into daphne-eu:main May 16, 2024
2 checks passed
@AlexRTer AlexRTer deleted the matrix-kernel branch June 25, 2024 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants