This is an archive of the discontinued LLVM Phabricator instance.

[mlir] GreedyPatternRewriteDriver: Ignore scope when rewriting top-level ops
ClosedPublic

Authored by springerm on Feb 2 2023, 1:05 AM.

Details

Summary

Top-level ModuleOps cannot be transformed with the GreedyPatternRewriteDriver since D141945 because they do not have an enclosing region that could be used as a scope. Make the scope optional inside GreedyPatternRewriteDriver, so that top-level ops can be processed when they are on the initial list of ops.

Note: This does not allow users to bypass the scoping mechanism by setting config.scope = nullptr.

Diff Detail

Event Timeline

springerm created this revision.Feb 2 2023, 1:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2023, 1:05 AM
springerm requested review of this revision.Feb 2 2023, 1:05 AM
mikeurbach accepted this revision.Feb 2 2023, 8:16 AM

Thanks! I think this makes sense to me.

This revision is now accepted and ready to land.Feb 2 2023, 8:16 AM

I'm curious about the use case here, wouldn't replacing the top-level op cause a crash? The pass manager and tools hold a pointer to the top level op, so it doesn't seem safe to apply rewrites to it.

mehdi_amini accepted this revision.Feb 2 2023, 11:23 AM

LGTM, should the description has Fixes #60462 as well?

mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
276

Technically you're doing more pointer chasing here aren't you? The op->getParentOp() will do some redundant traversal from op->getParentRegion().
This may very much not matter...

I'm curious about the use case here, wouldn't replacing the top-level op cause a crash? The pass manager and tools hold a pointer to the top level op, so it doesn't seem safe to apply rewrites to it.

Absolutely, but you can rewrite in place or rewrite downwards. For example a pattern may match a my_hw_specific_module op and normalize attribute, and lookup the entry point and the global variables and do something about it, without replacing the top-level my_hw_specific_module op.

Also, this is in line with a module pass: independently of the pattern-rewriting you can modify the ModuleOp in a Module pass but you can't replace it!

I'm curious about the use case here, wouldn't replacing the top-level op cause a crash? The pass manager and tools hold a pointer to the top level op, so it doesn't seem safe to apply rewrites to it.

Absolutely, but you can rewrite in place or rewrite downwards. For example a pattern may match a my_hw_specific_module op and normalize attribute, and lookup the entry point and the global variables and do something about it, without replacing the top-level my_hw_specific_module op.

Also, this is in line with a module pass: independently of the pattern-rewriting you can modify the ModuleOp in a Module pass but you can't replace it!

I see, I ran into an issue because dialect conversion includes the parent op in what can be matched: https://github.com/llvm/llvm-project/issues/60491
But I see now that this change only affects applyOpPatternsAndFold, and not applyPatternsAndFoldGreedily. Since applyOpPatternsAndFold is a more targeted API I guess it would be less likely to run into a similar issue.

This revision was landed with ongoing or failed builds.Feb 3 2023, 12:57 AM
This revision was automatically updated to reflect the committed changes.
springerm marked an inline comment as done.