[RFC] Split the `complex` dialect from `std`

This is part of splitting the std dialect.

Dialect Scope and Goal

The complex dialect is intended to hold complex numbers creation and arithmetic ops.

The following ops will be moved to ComplexOps.td:

  • std.create_complex -> complex.create
  • std.im -> complex.im
  • std.re -> complex.re
  • std.addcf -> complex.addf
  • std.subcf -> complex.subf

Examples of future additions to the complex dialect could be:

  • compex.conj
  • complex.divf
  • complex.mulf

Development process

The split will be done in two steps

  • Create complex dialect without removing complex ops from std
  • Remove complex ops from std after TensorFlow has migrated to the ones in complex

ComplexType allows integer element type. I am not aware if anyone would ever need that.

It isn’t clear to me why upstream development should be driven specifically to fit the need of TensorFlow in particular. I’m not fond of this kind of rational to drive upstream development in general and the kind of precedent this sets.

Otherwise +1 for the split overall!

That’s a good point. I’m assuming Alex meant to give downstream users time to update (not sure how much easier it would make it though as you have all the matching rules and C++ class usage, passes upstream and downstream that consumes or produces them, but perhaps there is some aliases or typedeffing that could simplify it). Perhaps just say a week/two weeks post split independent on whether who changed what where?

Me neither, it could be useful but I’m not sure anyone does (but I’m biased, I think of complex numbers as ordered pair of real numbers).

I’m not fond of the rationale phrasing but as someone with a couple of downstream projects, I do like the aim in sequencing large breaking changes incrementally.

Perhaps this can be fixed with a rephrase: Remove complex ops from std after giving downstream projects time to migrate (I work on tensorflow and we estimate that X weeks would be helpful as a compatibility window).

Absolutely! This is how I proceeded with the revamp of the dialect registration for example.

We shouldn’t put an expectation that every change will be done incrementally though: I’m supporting it only if it is easier to develop and review (or at least does not make it harder to achieve upstream).

This will only affect the listed Ops, correct? And not the type?

(Just checking. Thanks in advance.)

@schweitz Yes, the type won’t be affected.

1 Like

@stellaraccident Thanks for the rephrase!

@mehdi_amini Agree with everything above. Doing this split incrementally definitely won’t make it harder to develop or review in this particular case. But it would make life a bit easier for the downstream projects.

Would it make sense then to assume that the complex type always has real element type, unless explicitly specified? I would drop ‘f’ in the op names, i.e. complex.sub not complex.subf.

+1 to the splitting and @stellaraccident’s formulation of the compatibility window. I have implemented several breaking changes recently and found the following process convenient for everybody. (a) implement the replacement functionality in parallel to the existing functionality while keeping it in a separate namespace/dialect/library; this allows for proper review (multiple commits if necessary), testing and avoids multi-thousand line patches that are difficult to review. (b) switch all in-tree uses to the new functionality and deprecate the old one; the switch commits are mechanical and provide downstream users with good examples on how to perform the switch. (c) remove the deprecated functionality several weeks/months after deprecation; whoever is still using the deprecated functionality will be broken, but that is expected if one relies on deprecated stuff. IMO, this not only helps downstream, but also makes the changes smoother upstream by isolating functional changes from mechanical ones, removing the pressure to land, and making it easy to rollback/forward the switch commits only.

Mathematically, complex numbers are an extension of real numbers. There exist Gaussian integers, which are a similar extension of integers, but they seem to be used only in some parts of the number theory.

Thanks @pifon2a for driving this forward.

+1 to what @ftynse said. Should we formulate this as best practice somewhere, so we do not have to discuss this again? This would likely require splitting that aspect out into a process/community guidelines RFC of some kind.

As we do not have operations on the integer variant, I would be in favor of dropping integer support for now.

See https://mlir.llvm.org/getting_started/DeveloperGuide/#breaking-changes

Feel free to send me PR for wordsmith!

+1 in general. Though I think this change is literally just moving code from one place to another (so 100% mechanical) and it seems like overkill to go the “bring it up in parallel” route. At least that’s my opinion based on recently splitting the tensor dialect.