This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Support for compatible Dialect Builders
AcceptedPublic

Authored by AlexEichenberger on Jun 30 2021, 10:57 AM.

Details

Summary

We have currently one builder for a few common StandardOps operation in ArithBuilder. In order to facilitate the creation of one dialect builder from another one, this patch create a superclass, DialectBuilder that stores the builder and location. Specialized builders inherit from the superclass their data and can focus to primarily providing methods to generate code.

Say in the future we may have a LinAlg or SCF builder, we would then be able to initialize these other specialized builders from a current one, e.g.

SCFBuilder createSCF(createArith);

In addition, I added a subtract method to the current ArithBuilder.

Diff Detail

Event Timeline

AlexEichenberger requested review of this revision.Jun 30 2021, 10:57 AM
AlexEichenberger edited the summary of this revision. (Show Details)Jun 30 2021, 10:58 AM
rriddle requested changes to this revision.Jun 30 2021, 11:03 AM

What's the difference between this and just inheriting from ImplicitLocOpBuilder?

This revision now requires changes to proceed.Jun 30 2021, 11:03 AM

What's the difference between this and just inheriting from ImplicitLocOpBuilder?

My intent is to make the least amount of change to the ArithBuilder introduced by @nicolasvasilache. That struct is very lightweight and basically offers auto-completion and transparent argument typing.

@rriddle: you are suggesting that I make ArithBuilder a subclass of ImplicitLocOpBuilder? If that is your intend, I can prototype that solution as well.

What's the difference between this and just inheriting from ImplicitLocOpBuilder?

My intent is to make the least amount of change to the ArithBuilder introduced by @nicolasvasilache. That struct is very lightweight and basically offers auto-completion and transparent argument typing.

@rriddle: you are suggesting that I make ArithBuilder a subclass of ImplicitLocOpBuilder? If that is your intend, I can prototype that solution as well.

Yeah, I would suggest prototyping other approaches to see feasibility there. We should only be adding new utility classes to IR/ when an existing solution doesn't work (can't be reasonably adapted to work). Right now, the benefits of an explicit DialectBuilder class aren't clear to me.

Second prototype. There is one subtle difference: the old ArithBuilder had only a reference to a builder, so if the original builder was changed, it would change the one used by the ArithBuilder too. In this new version, ArithBuilder is a builder specialized for arithmetic ops, and thus changes to an original builder will not impact it's version.

This seems reasonable to me.

rriddle accepted this revision.Aug 3 2021, 9:51 PM

Can you update the patch description?

This revision is now accepted and ready to land.Aug 3 2021, 9:51 PM