Page MenuHomePhabricator

[mlir][sparse] add more unittest cases to sparse dialect merger

Authored by Peiming on Jun 17 2022, 7:31 AM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes


Peiming updated this revision to Diff 438055.Jun 17 2022, 3:39 PM

rollback change by mistake

Good work, I like the extension of our testing a lot!
A few nits on style.


why is this empty line gone? I like vertical space around the section-separating comments

(attached comments usually apply to code below directly)


+1, or use a FOR construct that Wren introduced in our library header (search for FOREVERY_ .... for inspiration)


add comment on new parameter


Make this a section separation, i.e.

/ Tests with all sparse inputs.

or, if this applies to the class, then attach, but add comment to MergerTest4T1L


remove this period, since sentence continues

Peiming updated this revision to Diff 438236.Jun 19 2022, 7:00 PM
This comment was removed by Peiming.
Peiming updated this revision to Diff 438694.Jun 21 2022, 7:16 AM

use FOREVERY_ marco to remove repeated code + improve comments

wrengr added inline comments.Jun 21 2022, 12:26 PM

I would rename this to FOREVERY_BINOP, since this is just for the binops not for Ops with other arities.


You should pass the second arguments as Kind::kFooT rather than just as kFooT. That way all the users of this x-macro can simply use the KIND parameter they're given rather than needing to say Kind::KIND. Any sort of code repetition like needing to say Kind::KIND over and over is a source of potential bugs from typos, and so should be avoided.


Macro parameters should also be uppercase. Ditto for all the other macros you define below

Peiming updated this revision to Diff 438845.Jun 21 2022, 2:57 PM
Peiming marked 4 inline comments as done.

Use uppercase for macro parameters

Peiming updated this revision to Diff 438849.Jun 21 2022, 3:15 PM
Peiming marked 3 inline comments as done.

Fix typo

Getting there! :) I have a lot of duplicate comments, but wanted to make sure you got them all before I do a final pass of reviewing


In C/C++ jargon, this isn't a "declaration" but rather a "definition"; so even though your macro's name makes sense colloquially, you might want to rename it to avoid confusion. In other places we use DECL_FOO for macros that declare something, and IMPL_FOO for macros that implement/define something.

...Though for this one I'm not sure IMPL_PATTERN is a terribly good name either. Perhaps IMPL_BINOP_PATTERN?


Same issue re "declare". Perhaps use IMPL_BINOP_ADDEXP?


I'm not terribly comfortable using the OP directly here since it seems likely to cause name clashing. Perhaps use OP##Expr instead?

(You could use OP##Exp to remain closer to the style in the rest of this file, though personally I'm not a fan of using "exp" to abbreviate "expression", since there's too much potential for confusion with "exp" meaning "exponential". Then again I've worked on a lot of compilers that do a lot of work with exponentials/logarithms.)


Should capitalize the macro parameter here too


I think it'd clean up this code considerably to float out variable bindings for auto p0 = tensorPattern(t0) and ditto for t1


To avoid repeating things here, You should break up the FOREVERY_BINOP x-macro into two separate macros: FOREVERY_DISJ_BINOP and FOREVERY_CONJ_BINOP. Then you can just call FOREVERY_DISJ_BINOP(MERGER_TEST_DISJ), where you update the MERGER_TEST_DISJ macro to take the extra KIND parameter and just ignore it.

For other places where you really do want to iterate over all the binops, you can define the old combined x-macro in terms of the two new ones: #define FOREVERY_BINOP(DO) FOREVERY_DISJ_BINOP(DO) FOREVERY_CONJ_BINOP(DO)

You can of course repeat the idea in order to split FOREVERY_DISJ_BINOP up into FOREVERY_COMMUTATIVE_DISJ_BINOP and FOREVERY_NONCOMMUTATIVE_DISJ_BINOP, in order to distinguish subtraction from addition/or as mentioned in your todo below. Or if commutativity isn't the most logical way to split things up, then you could instead do a three-way split into FOREVERY_ADD_BINOP, FOREVERY_OR_BINOP, and FOREVERY_SUB_BINOP.


Capitalize the OP parameter here too


capitalize macro arguments here too






ditto. Also, for consistency with the other tests you may want to rename them to CONJ1 and CONJ2 (or CONJ0 and CONJ1)






"opted" is a confusing and unnecessary abbreviation, just spell out "optimized"


ditto, though it's less of an issue here than in the previous few tests





Peiming updated this revision to Diff 439788.Jun 24 2022, 9:00 AM

Reduce code repetition, address comments from Wren

aartbik added inline comments.Jun 24 2022, 12:03 PM

This is very nitpicky (but this is how we are ;-), but I would like to see a consistent order on the type suffix.
We always use the order F,C,I


This too needs to make the bits/simple distinction clear in the doc, like L104


I think I have a slight preference for putting all FOR macros together, since you already define one up, since readers are typically searching for #defines at the top of the file. Perhaps you can even start this file with

Iteration macros.

#define FOR1 ...

Note that I only mean moving the FOR loops up. The actual IMPL macros and applying FOR to them should of course remain the in proper section, since it expands to code at that place

#define FOR2 ...



So, just to be clear, this define + FOR applied to it, then undefine, remains here.

aartbik added inline comments.Jun 24 2022, 12:05 PM

The online tool messed up my suggested code. Let me try with formatting

// Iteration macros.

#define FOR1 ...
#define FOR2 ...

// Next section
wrengr added inline comments.Jun 24 2022, 1:03 PM

I like the name IMPL_BINOP_EXPR :)


+1 for grouping all the FOREVERY x-macros together in one spot, and ideally all at the top of the file.

Remember, x-macros are defining groups of things that we always want to handle together. So it's important to give all the x-macros together in one spot so that readers can look at them and say "ah yes, these are the natural groupings of things" (e.g., conjunctive vs disjunctive things), so they know how to think about them before getting into the various places where the x-macros are called.


These two x-macros are redundant with the ones which take (TEST, OP). Remember, just because you pass OP to TEST doesn't mean that TEST actually needs to use it.

In fact, I'd reduce things further by combining the (TEST, OP) variants with the FOREVERY_BINOP xmacro. That is, you should only need to define:

#define FOREVERY_BINOP(DO, ...) \
    DO(mulf, Kind::kMulF, __VA_ARGS__) \
    DO(mulc, Kind::kMulC, __VA_ARGS__) \
    and so on and so forth

And then you can call it à la:

#define X(OP, KIND, FOO, BAR) \
   whatever you want to do
#undef X

Notably, FOREVERY_BINOP only provides the OP and KIND arguments of DO, and whatever else you pass into FOREVERY_BINOP will simply be passed along to DO. Which keeps FOREVERY_BINOP generic and not forced to know anything about the extra arguments that DO might want. On the other side, X isn't restricted to only taking in the OP and KIND parameters, but can take in whatever other parameters it might like. Moreover, by making the xmacro variadic like this, you can nest calls to xmacros (e.g., to handle the tests which have two or more ops); though it takes a little bit of boilerplate to do that.

Peiming updated this revision to Diff 439873.Jun 24 2022, 1:06 PM
Peiming marked 17 inline comments as done.

Address Aart's comments

Peiming updated this revision to Diff 439886.Jun 24 2022, 2:16 PM
Peiming marked 4 inline comments as done.

Further reduce code

Peiming updated this revision to Diff 439890.Jun 24 2022, 2:21 PM

Adding missing period..

Peiming added inline comments.Jun 24 2022, 2:46 PM

haha, will do!

If you look closer, you might find that they are already ordered as F,C,I (there is a kAndI in between of kMulI and kAddF), I will reordered it so that kAddI appears at the end of the list


Good Suggestions!


I am not sure whether I understand your point, since the (TEST, OP) macro is used to generate the production of two set (pairs of conj and disj, etc).

However, I do find that __VA_ARGS__ can help to remove those marcos, if it is not what you are referring to, just let me know!


I renamed them to FOREVERY_COMMON_XX_BINOP as they do not require special handling when doing pattern matching, etc. But I am not sure whether it is a good name.

Commutativity is not the logical way to split them up as matrix multiplication is also noncommunitive but it does not require special handling here.

Let me know if you have better name suggestion!

aartbik accepted this revision.Jun 24 2022, 3:33 PM

I am okay with the revision, but please wait for Wren's approval too, since she had a lot of feedback, and may have some more.

This revision is now accepted and ready to land.Jun 24 2022, 3:33 PM
wrengr accepted this revision.Jun 27 2022, 11:29 AM

The x-macros could still be cleaned up a bit, but I think it'd be easier to do myself than try to explain what I mean, so let's do that in a separate CL.

Other than that it's looking good! Once you remove the inline keyword, ship it! :)


All method definitions given within a class declaration are implicitly inline, so you don't need the keyword and should remove it.

Also, it's not clear to me why this is supposed to help improve readability of tests; but since this method (and comment) was there from before, we can address that in a separate CL.

Peiming added inline comments.Jun 27 2022, 12:33 PM

Oh, good to know!

Peiming updated this revision to Diff 440400.EditedJun 27 2022, 2:51 PM

remove inline

Peiming updated this revision to Diff 440408.Jun 27 2022, 3:06 PM

use full diff

Peiming marked 2 inline comments as done.Jun 27 2022, 3:10 PM

I'm seeing test warning/errors from this:

external/llvm-project/mlir/unittests/Dialect/SparseTensor/MergerTest.cpp:112:16: error: unused function 'subfPattern' [-Werror,-Wunused-function]


external/llvm-project/mlir/unittests/Dialect/SparseTensor/MergerTest.cpp:112:16: error: unused function 'subcPattern' [-Werror,-Wunused-function]
external/llvm-project/mlir/unittests/Dialect/SparseTensor/MergerTest.cpp:112:16: error: unused function 'subiPattern' [-Werror,-Wunused-function]
3 errors generated.

wrengr added a comment.Jul 1 2022, 4:11 PM

Should be able to fix that by adding LLVM_ATTRIBUTE_UNUSED from llvm/Support/Compiler.h. I'll send out a differential to do so

wrengr added a comment.Jul 1 2022, 4:21 PM

@brooksmoses Just posted D129027. (Am currently building it to see if any CMake changes are necessary)

@wrengr : Looks fixed; thanks!