Page MenuHomePhabricator

[mlir] Refactor AbstractOperation and OperationName
ClosedPublic

Authored by rriddle on Nov 16 2021, 6:07 PM.

Details

Summary

The current implementation is quite clunky; OperationName stores either an Identifier
or an AbstractOperation that corresponds to an operation. This has several problems:

  • OperationNames created before and after an operation are registered are different
  • Accessing the identifier name/dialect/etc. from an OperationName are overly branchy
    • they need to dyn_cast a PointerUnion to check the state

This commit refactors this such that we create a single information struct for every
operation name, even operations that aren't registered yet. When an OperationName is
created for an unregistered operation, we only populate the name field. When the
operation is registered, we populate the remaining fields. With this we now have two
new classes: OperationName and RegisteredOperationName. These both point to the
same underlying operation information struct, but only RegisteredOperationName can
assume that the operation is actually registered. This leads to a much cleaner API, and
we can also move some AbstractOperation functionality directly to OperationName.

Diff Detail

Event Timeline

rriddle created this revision.Nov 16 2021, 6:07 PM
rriddle requested review of this revision.Nov 16 2021, 6:07 PM

In a followup I plan to rename OperationName/RegisteredOperationName to OperationInfo/RegisteredOperationInfo (given that these are actually closer to what they represent). I also want to use OperationName instead of Identifier in a few places now that the created before registration != created after registration problem is fixed.

mehdi_amini added inline comments.Nov 16 2021, 9:31 PM
mlir/include/mlir/IR/OperationSupport.h
102

That seems redundant with the StringAttr, isn't it?

121

Aren't Identifier replaced with StringAttr now?

rriddle added inline comments.Nov 16 2021, 9:35 PM
mlir/include/mlir/IR/OperationSupport.h
102

Yes, but I've delayed removing it until I can see if there is a performance hit from removing this. There are a few other cleanups that I also want to make in this area (e.g. inlining traits array) in followups.

121

Thanks for the catch, I hadn't rebased on the Identifier reference removal yet.

mehdi_amini accepted this revision.Nov 16 2021, 9:38 PM
This revision is now accepted and ready to land.Nov 16 2021, 9:38 PM
This revision was automatically updated to reflect the committed changes.
rriddle marked 2 inline comments as done.