This is an archive of the discontinued LLVM Phabricator instance.

Add a mechanism for Dialects to provide a fallback for OpInterface
ClosedPublic

Authored by mehdi_amini on Dec 10 2020, 9:21 PM.

Details

Summary

This mechanism makes it possible for a dialect to not register all
operations but still answer interface-based queries.
This can useful for dialects that are "open" or connected to an external
system and still interoperate with the compiler. It can also open up the
possibility to have a more extensible compiler at runtime: the compiler
does not need a pre-registration for each operation and the dialect can
inject behavior dynamically.

Diff Detail

Event Timeline

mehdi_amini created this revision.Dec 10 2020, 9:21 PM
mehdi_amini requested review of this revision.Dec 10 2020, 9:21 PM

Remove interfacemap

I forget, what was River's suggestion to make this more efficient?

mlir/include/mlir/IR/OpDefinition.h
1747

Nit:

if (auto *opIface = ...) return

to reduce scope (and not used in early return)

mlir/test/lib/Dialect/Test/TestDialect.cpp
177

If this was

return op->getName().getStringRef() == "test.unregistered_side_effect_op";

would that also restrict it to this op?

181

Why is ::mlir needed?

204

Instantiate

206

Is there a reason why not unique_ptr?

223

So for every op, one would have an entry here? Probably up to dialect to implement in efficient way for the fallback behavior they want.

I forget, what was River's suggestion to make this more efficient?

There wasn't an efficiency aspect left, the idea River had was about generating a different base class with TableGen so that the subclass implementation for the interface fallback could be friendlier to write.

I forget, what was River's suggestion to make this more efficient?

There wasn't an efficiency aspect left, the idea River had was about generating a different base class with TableGen so that the subclass implementation for the interface fallback could be friendlier to write.

So just convenience generation pending?

I forget, what was River's suggestion to make this more efficient?

There wasn't an efficiency aspect left, the idea River had was about generating a different base class with TableGen so that the subclass implementation for the interface fallback could be friendlier to write.

So just convenience generation pending?

Yes.
I plan to get back to this in order to prototype a new TF control dialect.

mehdi_amini marked 4 inline comments as done.

Address comments

mlir/test/lib/Dialect/Test/TestDialect.cpp
177

Yes, I updated the code to make it more clear.

181

Nothing, copy/pasted from somewhere else.

Removed!

206

It is a void*, making it a unique_ptr would require to declare some private classes. I'd expect something not in the "TestDialect" to use another more structured solution.

223

Yes!

jpienaar accepted this revision.Mar 9 2021, 5:34 PM

Nice, thanks

This revision is now accepted and ready to land.Mar 9 2021, 5:34 PM
rriddle accepted this revision.Mar 9 2021, 5:44 PM

Can you add documentation for this?

mlir/include/mlir/IR/OpDefinition.h
1750

nit: Please use complete sentences in comments.

1751–1752
mlir/test/lib/Dialect/Test/TestDialect.cpp
168

Functions should be static and top-level.

206

You only have to forward declare them. You could even put the class in the dialect declaration:

// in .td
  let extraClassDeclaration = [{
    ...
  private:
    class CustomFallbackInterface;

    // Storage for a custom fallback interface.
    std::unique_ptr<CustomFallbackInterface> fallbackEffectOpInterfaces;
  }];
// in .cpp
class TestDialect::CustomFallbackInterface : public TestEffectOpInterface::Model<TestOpEffectInterfaceFallback> {};
mlir/tools/mlir-tblgen/DialectGen.cpp
147

You need to add ::mlir:: to TypeID and Operation to not break out-of-tree projects.

mehdi_amini retitled this revision from Prototype OpInterface fallback to Dialects to Add a mechanism for Dialects to provide a fallback for OpInterface.
mehdi_amini edited the summary of this revision. (Show Details)

Use the TableGen mechanism

mehdi_amini marked 5 inline comments as done.

Address River's comment

mlir/test/lib/Dialect/Test/TestDialect.cpp
206

It won't work because the constructor body is in the header...

Add some doc in Interfaces.md

rriddle added inline comments.Mar 19 2021, 7:50 PM
mlir/docs/Interfaces.md
237 ↗(On Diff #332061)

Do we need to pass in the operation here? Or can we just pass in the OperationName instead? We have several few places within the code base that may try to use the AbstractOperation to check for interfaces without having a concrete operation instead, it would be great not to lose that ability.

mlir/include/mlir/IR/OpDefinition.h
1755

Drop the getIdentifier here?

rriddle requested changes to this revision.Mar 19 2021, 7:50 PM

(Marking as requested changes while discussing updated patch)

This revision now requires changes to proceed.Mar 19 2021, 7:50 PM

Fix doc: the fallback applies also to registered operations

mehdi_amini marked an inline comment as done.

Use an Identifier instead of an Operation* to retrieve the interface

mlir/docs/Interfaces.md
237 ↗(On Diff #332061)

Yes the name should be enough here. The original version of the patched needed the operation here because it was captured in the returned instance IIRC.

rriddle added inline comments.Mar 22 2021, 5:17 PM
mlir/docs/Interfaces.md
237 ↗(On Diff #332061)

Why Identifier and not OperationName?

rriddle accepted this revision.Mar 22 2021, 5:33 PM

Only comment is on OperationName vs Identifier, but otherwise LGTM.

This revision is now accepted and ready to land.Mar 22 2021, 5:33 PM

Use OperationName

This revision was landed with ongoing or failed builds.Mar 24 2021, 1:41 AM
This revision was automatically updated to reflect the committed changes.

Nice feature! I have one small question. Can this fallback OpInterface be represented as DialectInterface itself? In that case it can be registered in the Dialect via addInterfaces method without manual memory management (destructor overload or unique_ptr).

struct TestOpEffectInterfaceFallback
    : public DialectInterface::Base<TestEffectOpInterface::FallbackModel<TestOpEffectInterfaceFallback>> {
  ...
};


void TestDialect::initialize() {
  addInterfaces<TestOpEffectInterfaceFallback>();
}

void *TestDialect::getRegisteredInterfaceForOp(TypeID typeID,
                                               OperationName opName) {
  if (opName.getIdentifier() == "test.unregistered_side_effect_op" &&
      typeID == TypeID::get<TestEffectOpInterface>())
    return getRegisteredInterface<TestOpEffectInterfaceFallback>();
  return nullptr;
}

That's an excellent remark!
I intentionally did it this way because in my original mock-up I wanted to be able to initialize the interface instance with some state and keep it around. But it may not be warranted actually.