Page MenuHomePhabricator

[mlir] Make CSE available as utility function
AbandonedPublic

Authored by Hardcode84 on Mar 14 2022, 10:17 AM.

Details

Reviewers
rriddle
jpienaar
Summary

Also, provide user hook for hasSideEffects.

Diff Detail

Event Timeline

Hardcode84 created this revision.Mar 14 2022, 10:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2022, 10:17 AM
Hardcode84 requested review of this revision.Mar 14 2022, 10:17 AM
rriddle requested changes to this revision.Mar 14 2022, 10:21 AM

This is missing some context/justification.

This revision now requires changes to proceed.Mar 14 2022, 10:21 AM

It needed for CSE passes with some custom side-effect logic.

rriddle added a comment.EditedMar 14 2022, 10:29 AM

It needed for CSE passes with some custom side-effect logic.

What custom logic? Is this not something that can be generalized in a way that can be hooked into by the CSE pass transparently? More complicated side effect analysis is under-developed upstream. I have a general hesitance to "extend" passes via callbacks, because it often means that the core component will stagnate any evolution (we now have N cse passes instead of 1). This is also now widely exposing CSE as a utility, which isn't something that I would generally recommend users to call inside of their passes (because we now end up with mega passes).

  1. I have a custom "mydialect.noSideEffect" attribute, which I can attach to any op (e.g. CallOp) and I want CSE to always simplify them
  2. I want to write a pass which CSE loads from constant data (which is not strictly MemoryEffectOpInterface::hasNoEffect)
  1. I have a custom "mydialect.noSideEffect" attribute, which I can attach to any op (e.g. CallOp) and I want CSE to always simplify them

Wouldn't it be better to standardize an attribute for this instead?

  1. I want to write a pass which CSE loads from constant data (which is not strictly MemoryEffectOpInterface::hasNoEffect)

I would expect that these loads operation can implement the MemoryEffectOpInterface and returns their side-effect based on the same logic as what you're trying to do with the callback?

Wouldn't it be better to standardize an attribute for this instead?

It would be nice to have such attribute upstream but I am not ready for such endeavor :)

I would expect that these loads operation can implement the MemoryEffectOpInterface and returns their side-effect based on the same logic as what you're trying to do with the callback?

I'm not sure this is a good idea, MemoryEffectOpInterface is also used in other places and I don't know what effect it will cause there (especially if I change behavior of upstream op). Another issue that it won't work if I want to CSE upstream op but with some knowledge only available in my compiler.

Wouldn't it be better to standardize an attribute for this instead?

It would be nice to have such attribute upstream but I am not ready for such endeavor :)

There are various tradeoffs to how we generalize/expose components upstream. We have to be careful about how we expose components, and generally I prefer to evolve the upstream
infra to support use cases instead of opening them up freely to callbacks. It has the trade off of forcing things to be designed/pushed upstream, but that creates a better system
IMO (we wouldn't have nearly as many components available upstream without it).

I would expect that these loads operation can implement the MemoryEffectOpInterface and returns their side-effect based on the same logic as what you're trying to do with the callback?

I'm not sure this is a good idea, MemoryEffectOpInterface is also used in other places and I don't know what effect it will cause there (especially if I change behavior of upstream op). Another issue that it won't work if I want to CSE upstream op but with some knowledge only available in my compiler.

Kind of with the above. MemoryEffectOpInterface (and surrounding components) are the intended way we have/want to support these types of things upstream. There has been a long
standing TODO to support CSEing memory operations, and this will generally involve evolving/adding additional functionality on top of these interfaces.

I'm not sure this is a good idea, MemoryEffectOpInterface is also used in other places and I don't know what effect it will cause there (especially if I change behavior of upstream op).

I'm not sure I totally follow? What kind of "effect" are you afraid it would cause? Seems like to me that right now the interface isn't fully implemented with the ops you're looking for?

Ultimately the memory effect interface is intended to do exactly this! The whole point of developing this interface was exactly to avoid this kind of callback to customize passes' behavior.

Hardcode84 abandoned this revision.Mar 15 2022, 2:34 PM

For downstream projects it can be more convenient to have this hook so they can experiment without pushing fundamental global changes upstream (like changing behavior of existing ops, which may or may not be a good idea), but I understand your point.
For now I will just copypaste this entire pass to my project and do necessary changes locally.