metagraph-dev / mlir-graphblas

MLIR tools and dialect for GraphBLAS
https://mlir-graphblas.readthedocs.io/en/latest/
Apache License 2.0
15 stars 6 forks source link

DWIM rewrite patterns for graphblas.reduce_to_vector and matrix multiply ops #239

Closed paul-tqh-nguyen closed 2 years ago

paul-tqh-nguyen commented 2 years ago

This PR also:

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 1482839153


Totals Coverage Status
Change from base Build 1479560165: 0.0%
Covered Lines: 3884
Relevant Lines: 4594

💛 - Coveralls
jim22k commented 2 years ago

I had an internal dialog with myself -- capturing here for others to consider.

I had the thought that DWIM rewriting should happen on the generic versions of operations. That way anyone who manually writes a generic call (useful for user-defined operators) will get the benefit of DWIM rewriting.

I have decided this is a bad choice because several operations have custom operators which don't go through the generic machinery. For example, ReduceToVector with count doesn't lower to generic. Instead, it writes the full lowering pass directly (because it is both easy to write and much more efficient than going through the generic looping). For the case of ReduceToVector, that would require DWIM rewriting to handle both ReduceToVector and ReduceToVectorGeneric. That is asking too much of DWIM rewriting.

So, I think we should only perform DWIM rewriting on the non-generic operations. That should cover the 99% of normal cases, giving users freedom in writing algorithms without worrying about the strict requirements of the lowering algorithm. And for those users who need to manually write generic operations, they will have to operate in strict mode. But they are already "expert" users, so this is not much of a burden.