Page MenuHomePhabricator

[MLIR] Add OpTrait::DominanceFreeScope
Needs RevisionPublic

Authored by stephenneuendorffer on Wed, May 20, 10:31 PM.



Some dialects have semantics which does is not well represented by
common SSA structures with dominance constraints. This patch allows
operations to declare that they are 'dominance-free', indicating that
their contained regions are not requires to meet the dominance

see discussion here:

Diff Detail

Event Timeline

stephenneuendorffer edited the summary of this revision. (Show Details)
lattner accepted this revision.Thu, May 21, 2:46 PM

Please fix the lint warnings, but otherwise this LGTM as a step. Thank you for driving this!

The primary problem with this patch is that it won't work with MLIR files that don't have registered ops. To fix this, we need to either:

  1. invert the polarity of the flag - making dominance checking "opt-in", or
  2. Have specific "verbose" syntax for unregistered ops that reflects this property, and a bit in Operation that tracks this (because the trait won't exist if the op isn't registered).

The discussion about the tradeoffs of these are involved, so we should probably do that async with landing this patch.

River do you agree?

mehdi_amini requested changes to this revision.Thu, May 21, 7:30 PM

FYI, River is OOO and back next Wednesday, and I'd *really* want to have him sign-off on this.

Concretely on this patch, while the implementation is not very intrusive, there are a few things I'd like to see discussed/addressed/planned:

  • the documentation should be carefully updated: the code says " For more details, see" but isn't there?
  • what is the status of the passes in MLIR? What would break when there is a DominanceFreeScope? Are we gonna update the infra to check for this property everywhere? The conversion framework expects operands to be visited before we convert an op for example.
  • There is the syntactic/structural property of dominance that you relax here, piggybacking on the "unreachable blocks" relaxed semantics. However the definition of a basic block is that operation are executed in sequence: I am not sure what are the implication here about this?

(I'll post this in the thread as well, maybe it is better to discuss there)

This revision now requires changes to proceed.Thu, May 21, 7:30 PM