Page MenuHomePhabricator

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

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

Merge BIN_PATTERN and BIN_EXP

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.

mlir/unittests/Dialect/SparseTensor/MergerTest.cpp
64

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

(attached comments usually apply to code below directly)

130–138

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

151–153

add comment on new parameter

286

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

377–378

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
mlir/unittests/Dialect/SparseTensor/MergerTest.cpp
105–110

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

106–117

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.

119

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

mlir/unittests/Dialect/SparseTensor/MergerTest.cpp
119

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?

133

Same issue re "declare". Perhaps use IMPL_BINOP_ADDEXP?

134

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.)

384–409

Should capitalize the macro parameter here too

391–406

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

409–413

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.

417–627

Capitalize the OP parameter here too

451

capitalize macro arguments here too

504

ditto

513

ditto

569

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

578

ditto

612

ditto

612

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

619

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

654

ditto

654

ditto

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
mlir/unittests/Dialect/SparseTensor/MergerTest.cpp
106

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

192

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

369

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 ...

etc.

384–409

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
mlir/unittests/Dialect/SparseTensor/MergerTest.cpp
369

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

//
// Iteration macros.
//

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

//
// Next section
//
wrengr added inline comments.Jun 24 2022, 1:03 PM
mlir/unittests/Dialect/SparseTensor/MergerTest.cpp
133

I like the name IMPL_BINOP_EXPR :)

369

+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.

372–386

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
FOREVERY_BINOP(X, foo, bar)
#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.

https://en.wikipedia.org/wiki/X_Macro
https://gcc.gnu.org/onlinedocs/cpp/Variadic-Macros.html

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
mlir/unittests/Dialect/SparseTensor/MergerTest.cpp
106

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

369

Good Suggestions!

372–386

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!

409–413

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! :)

mlir/unittests/Dialect/SparseTensor/MergerTest.cpp
144

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
mlir/unittests/Dialect/SparseTensor/MergerTest.cpp
144

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]
FOREVERY_BINOP(IMPL_BINOP_PATTERN)

^

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!