Page MenuHomePhabricator

[MLIR] Add convenience specializations of OpBuilder create() and friends
AbandonedPublic

Authored by jurahul on Jul 17 2020, 3:54 PM.

Details

Summary
  • Add template specialization of OpBuilder::create() and similar functions in PatternRewriter to allow calling create without having to cast collective arguments to ArrayRef<> of the corresponding type.

Diff Detail

Event Timeline

jurahul created this revision.Jul 17 2020, 3:54 PM

I'm not really convinced that this is a win given the use of macros.

mlir/include/mlir/IR/Builders.h
14

Please remove the use of macros, this is highly polluting of all namespaces and is especially bad because this is such a highly included file.

Note, there can be more specializations that can be added, but I wanted to get this out to get some feedback. Some next items to handle are (1) canonicalize various CallOp build parameters (some have function followed by result types, and other result types followed by functions) and then add a specialization for that, and (2) one for branch instructions. The goal is to make the calls to create/replace etc slightly simpler and eliminate/reduce the need to type ArrayRef<Type> or ArrayRef<Value> in front of a collection of types or values passed into these functions.

jurahul marked an inline comment as done.Jul 17 2020, 4:04 PM
jurahul added inline comments.
mlir/include/mlir/IR/Builders.h
14

I was concerned as well here, other option is to not use macro's and do this manually (which is what I had done). Assuming a small set of specializations may cover a lot of ground, may be worth concerning. I know typing ArrayRef<>{} does not sound that bad, but I think if we could remove this noise, it should help readability slightly.

rriddle added inline comments.Jul 17 2020, 4:06 PM
mlir/include/mlir/IR/Builders.h
14

Slightly related, but we need to move the uses of ArrayRef<Type>/ArrayRef<Value> -> TypeRange/ValueRange which are also slightly easier to type.

jurahul marked an inline comment as done.Jul 17 2020, 4:45 PM
jurahul added inline comments.
mlir/include/mlir/IR/Builders.h
14

Yes, TypeRange and ValueRange is shorter, but not by that much. I was hoping to avoid typing either. It seems all the builders need to move to use TypeRange and ValueRange regardless (which will be a separate change, likely before something like this change).

Unless I am missing some C++ feature, I am assuming the alternative to macros is manually writing them out.

jurahul marked an inline comment as done.Jul 17 2020, 5:38 PM
jurahul added inline comments.
mlir/include/mlir/IR/Builders.h
14

Filed https://bugs.llvm.org/show_bug.cgi?id=46769 to track the use of ValueRange and TypeRange in the build methods.

jurahul updated this revision to Diff 279252.Jul 20 2020, 8:08 AM

Eliminate macros

This is my attempt to remove the use of Macros. With this, we essentially need one specialization class for each function we want to specialize. I tried to go down a different route as well, creating an instance of an (empty) specialization object with a templated () operator. The hope was that we can define this class just once, and say the builder has an instance of this class called 'create' and we could do something like

builder.create<Scf::Yield>(...);

and hoping the templated operator() gets called. However, it seems C++ does not allow this directly for a templated operator() and the syntax would be:

builder.create.operator()<Scf::Yield>(...)

which does not look great. So the fallback is this method. We would probably want to move these specialization classes into their owe header so at least when someone adds a new specialization, all the specializations are in a single file, so they could all be updated together. The drawback is that we will still need another copy for each new function name that we want to specialize (like createChecked etc).

jurahul updated this revision to Diff 279259.Jul 20 2020, 8:23 AM

Cleanup leftover macro code

IMO, sometimes an explicit TypeRange() is better than {}. But I am supportive of the effort to reduce the verbosity of builder API.


On a slightly related note, some time ago, I posted a longish text reflecting on the evolution on builders. One of the ideas was to generate non-template builder functions in per-dialect classes as follows

struct SCFBuilder {
  YieldOp Yield(Location loc, ValueRange operands) { return b.build<YieldOp>(loc, operands); }
  ForOp For(Location loc, Value lb, Value ub, Value step) { return b.build<ForOp>(loc, lb, ub, step); }

  OpBuilder &b;
};

and use these as

OpBuilder builder(...);
SCFBuilder scf(builder);
scf.For(loc, lb, ub, step);
/* ... */
scf.Yield(loc, {operand, operand});

More information in the DSL-style builders section of https://llvm.discourse.group/t/evolving-builder-apis-based-on-lessons-learned-from-edsc/879.

This was also addressing the issue with verbosity of builder API and achieves what you seem to be shooting for here: remove the mix of function overloading and variadic templates that suppress some implicit casting rules in a non-trivial way. (There was one time when I literally had to look into clang internals to understand how overload resolution works in presence of template argument packs, and it's a particular kind of hell.) It also has some nice effects like working auto-complete with documentation, but also some drawbacks, in particular storing the OpBuilder in potentially multiple dialect specific-builders. May be it is worth exploring again.

Agreed, in certain instances if clarity is desired, an explicit TypeRange{} can be still be added. But in many cases, things may be clear enough that the extra 'TypeRange{}' or ValueRange{} may be just noise, which is what I am attempting to address here. We have some instances within MLIR, but I suspect several out-of-tree instances that would read easier without these casts.

I'll go over your discourse thread, but something like builder.createFor()... instead of builder.create<SCF::For> sounds interesting as well, and I agree that it can also address this issue of verbosity.

@ftynse, I read your document a bit, but another idea came to my mind which seems like an incremental change. Instead of the per-dialect builder you had suggested:

OpBuilder builder(...);
SCFBuilder scf(builder);
scf.For(loc, lb, ub, step);
/* ... */
scf.Yield(loc, {operand, operand});

what if in addition to the build() methods, ODS framework defines a static create method corresponding to each of the build methods? In that case, the code above will look like:

OpBuilder builder(...);
scf::YieldOp::create(builder, loc, {operand, operand});

This should be doable without a dialect specific builder and breaking any existing API. For generality, internally create() will just do what it does today, call:builder.create<YieldOp>(...); For what I am attempting to to, in addition to create, we may also want to define other create-like methods like createOrFold, replaceOpWithNewOp() etc. It seems emitting them on the ODS side should be straightforward and easily extensible (without needing redundant C++ specialization classes for each methods that needs specialization). I think this will still retain some of the advantages you mentioned earlier (auto-correct) without the drawback of a new builder class and carrying around the OpBuilder. Please let me know what you think, and the change would be incremental in nature.

@jurahul let's factor out this discussion to the forum, I don't think enough people follow individual review threads.

One of the issues EDSC builders try to solve is the verbosity and boilerplateness of the API. Having Dialect::Op::create(builder, loc, ...) looks equally, if not more, verbose than the existing builder, so it will likely end up as the third way of constructing ops (in addition to Builder.create and EDSC).

mravishankar resigned from this revision.Aug 3 2020, 11:36 PM
jurahul abandoned this revision.Aug 19 2020, 6:32 PM