This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Canonicalizer constructor should accept disabled/enabled patterns
ClosedPublic

Authored by Mogball on Dec 20 2021, 12:58 PM.

Details

Summary

There is no way to programmatically configure the list of disabled and enabled patterns in the canonicalizer pass, other than the duplicate the whole pass. This patch exposes the disabledPatterns and enabledPatterns options.

Diff Detail

Event Timeline

Mogball created this revision.Dec 20 2021, 12:58 PM
Mogball requested review of this revision.Dec 20 2021, 12:58 PM
Mogball edited the summary of this revision. (Show Details)Dec 20 2021, 12:58 PM

Does this work in non-debug builds? I don't remember if pattern names weren't just a debugging thing? It probably should be reflected in the name if we want to expose this publicly.

Mogball added a comment.EditedDec 20 2021, 1:04 PM

It apparently does. See https://github.com/llvm/llvm-project/blob/main/mlir/test/Transforms/test-canonicalize-filter.mlir

By default, the "debugName" of a pattern is set to its type name, even in non-debug builds

It apparently does. See https://github.com/llvm/llvm-project/blob/main/mlir/test/Transforms/test-canonicalize-filter.mlir

By default, the "debugName" of a pattern is set to its type name, even in non-debug builds

OK, that is still a "debugName" which I'm wary of providing more expectation than that on, so we should still reflect this in the API.

OK, that is still a "debugName" which I'm wary of providing more expectation than that on, so we should still reflect this in the API.

Sure, sounds good.

Mogball updated this revision to Diff 395518.Dec 20 2021, 1:38 PM

Update description of API to match that of FrozenRewritePatternSet.

@mehdi_amini I copied the description over from FrozenRewritePatternSet,
which is actually where the capability comes from. Based on how this was
set up in https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/Rewrite/PassUtil.td,
I think this is intended to be a somewhat commonly-used API.

Herald added a project: Restricted Project. · View Herald TranscriptDec 20 2021, 1:38 PM

It is possible to have pattern with empty name (when added using function pointer api).

Update description of API to match that of FrozenRewritePatternSet.

@mehdi_amini I copied the description over from FrozenRewritePatternSet,
which is actually where the capability comes from. Based on how this was
set up in https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/Rewrite/PassUtil.td,
I think this is intended to be a somewhat commonly-used API.

Well the issue may be instead into how it is presented in FrozenRewritePatternSet: this does not match the fact that patterns can have naming collision or no name at all (as Ivan indicated).

Update description of API to match that of FrozenRewritePatternSet.

@mehdi_amini I copied the description over from FrozenRewritePatternSet,
which is actually where the capability comes from. Based on how this was
set up in https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/Rewrite/PassUtil.td,
I think this is intended to be a somewhat commonly-used API.

Well the issue may be instead into how it is presented in FrozenRewritePatternSet: this does not match the fact that patterns can have naming collision or no name at all (as Ivan indicated).

That's true. I can update the comment over there too

Mogball updated this revision to Diff 395558.Dec 20 2021, 5:18 PM

Make clear that the labels used to filter patterns are debug names, and may be empty.

mehdi_amini accepted this revision.Dec 20 2021, 6:24 PM
This revision is now accepted and ready to land.Dec 20 2021, 6:24 PM