Page MenuHomePhabricator

[mlir][test] Shard and reorganize the test dialect
Needs ReviewPublic

Authored by Mogball on Jun 10 2022, 11:56 AM.

Details

Reviewers
ftynse
rriddle
Summary

Shard the test dialect by 4. This patch also reorganizes the manually-written
op hooks into TestOpDefs.cpp and format custom directive parser and printers
into TestFormatUtils, adds missing comment blocks, and moves around where
generated source files are included for types, attributes, enums, etc.

In my case, the compilation time of the test dialect drops from >60s to ~10s.

Depends on D127521

Diff Detail

Event Timeline

Mogball created this revision.Jun 10 2022, 11:56 AM
Mogball requested review of this revision.Jun 10 2022, 11:56 AM
rriddle requested changes to this revision.EditedJun 10 2022, 12:00 PM

The sharding here feels very arbitrary. Can we instead separate the operations based on what they are testing? That feels like a much better cleanup, and a better end state than sharding.

This revision now requires changes to proceed.Jun 10 2022, 12:00 PM

The sharding here feels very arbitrary. Can we instead separate the operations based on what they are testing? That feels like a much better cleanup, and a better end state than sharding.

I should clarify that I understand the utility of adding operation sharding support in general, but I'm not yet convinced that it's the best thing to do for the test dialect. The test dialect is monolithic only because of legacy reasons (it just grew over time), I expect that a much more maintainable and cleaner direction would be to actually split the different concepts being tested.

You mean moving ops into different dialects?

I foresee sadness and pain

You mean moving ops into different dialects?

I foresee sadness and pain

No, I'm talking about having different .td files for different groups of operations.

I agree with the fact that the test dialect should be sharded based on features instead of being a monolithic definition!

That said we need the sharding for our internal out-of-tree dialects still so it is a useful addition independently :)
(and the test dialect is just used to test the sharding feature)

To come back to the patch: what I'd like is the ability to change the sharding count in the CMake file without touching any source code anywhere. I just talked to Jeff about this and I think he has a solution for this!

I agree with the fact that the test dialect should be sharded based on features instead of being a monolithic definition!

That said we need the sharding for our internal out-of-tree dialects still so it is a useful addition independently :)
(and the test dialect is just used to test the sharding feature)

To come back to the patch: what I'd like is the ability to change the sharding count in the CMake file without touching any source code anywhere. I just talked to Jeff about this and I think he has a solution for this!

That would be really nice!

Although the test dialect should have its ops split up based on what they're testing (and further sharded with the tool if necessary), a lot of the ops in the test dialect are really arbitrary and there is probably no way to cleanly split the ops. A lot of test dialect ops are "multi-purpose" in that: "Oh, I need to test a region trait. Look, there's an op with a region! Let me just stick it on that one".

Mogball updated this revision to Diff 436271.Jun 12 2022, 9:57 PM

Automatic sharding