This is an archive of the discontinued LLVM Phabricator instance.

[mlir][IR] RewriterBase: Use composition instead of inheritance for Listener
ClosedPublic

Authored by springerm on Feb 5 2023, 4:56 AM.

Details

Summary

This change makes RewriterBase symmetric to OpBuilder.

  OpBuilder           OpBuilder::Listener
      ^                        ^
      |                        |
RewriterBase        RewriterBase::Listener
  • Clients can listen to IR modifications with RewriterBase::Listener.
  • RewriterBase no longer inherits from OpBuilder::Listener.
  • Only a single listener can be registered at the moment (same as OpBuilder).

RFC: https://discourse.llvm.org/t/rfc-listeners-for-rewriterbase/68198

Diff Detail

Event Timeline

springerm created this revision.Feb 5 2023, 4:56 AM
Herald added a project: Restricted Project. · View Herald Transcript
springerm requested review of this revision.Feb 5 2023, 4:56 AM
springerm updated this revision to Diff 495014.Feb 6 2023, 12:39 AM
springerm edited the summary of this revision. (Show Details)

update

springerm retitled this revision from [mlir][WIP] RewriterBase: Use composition instead of inheritance for Listener to [mlir] RewriterBase: Use composition instead of inheritance for Listener.Feb 6 2023, 12:39 AM
springerm added a reviewer: mehdi_amini.
springerm added a reviewer: ftynse.
rriddle added inline comments.Feb 6 2023, 9:47 AM
mlir/include/mlir/IR/Builders.h
291

Why does this have classof as a base class?

mlir/lib/IR/PatternMatch.cpp
219–224

Generally we hide this complexity in a CRTP base class, can you do that for Listener too? Generally top-level classes shouldn't be mucking around with TypeID directly.

springerm marked 2 inline comments as done.Feb 7 2023, 1:19 AM
springerm added inline comments.
mlir/include/mlir/IR/Builders.h
291

Not needed, deleted.

mlir/lib/IR/PatternMatch.cpp
219–224

Added a new base class to store the "kind".

I don't know what's the best way to use CRTP here. The problem is that we have a deep class hierarchy with user-derived classes. E.g.:

  ListenerBase
       ^
       |
OpBuilder::Listener
   ^    ^
   |    |
   |  MyBuilderListener (user-derived class)
   |
RewriterBase::Listener
        ^
        |
      MyRewriterBaseListener (user-derived class)

If ListenerBase<T> is CRTP-templatized, there must be a way to pass RewriterBase::Listener as a template argument to ListenerBase<T>. So OpBuilder::Listener must also be templatized. That would bloat up the API because users have to pass a template arg when creating their own OpBuilder::Listener subclass.

Another issue is with RewriterBase::Listener::classof. If I understand correctly, you were thinking of something like:

template <typename T>
ListenerBase<T>::classof(ListenerBaseBase *listener) {
  return TypeID::get<T> == listener->getTypeID();
}

This will probably not work for user-derived classes because TypeID(MyRewriterBaseListener) != TypeID(RewriterBase), so we wouldn't be able to cast MyRewriterBaseListener to RewriterBase::Listener. Also, I need to add yet another superclass ListenerBaseBase.

I changed the TypeID field to Kind. That captures it more accurately because user-derived classes have the same "kind" as their superclasses. Overall, this design is a bit like Value/ValueImpl: E.g., BlockArgumentImpl also has a classof function that checks the kind field.

springerm updated this revision to Diff 495417.Feb 7 2023, 1:19 AM
springerm marked 2 inline comments as done.

address comments

Friendly ping. This change would help us clean up some our code.

springerm retitled this revision from [mlir] RewriterBase: Use composition instead of inheritance for Listener to [mlir][IR] RewriterBase: Use composition instead of inheritance for Listener.Feb 14 2023, 3:50 AM
This revision is now accepted and ready to land.Feb 20 2023, 9:42 AM

River can you confirm you don't have any other issues with this?

This revision was landed with ongoing or failed builds.Feb 22 2023, 12:18 AM
This revision was automatically updated to reflect the committed changes.

River can you confirm you don't have any other issues with this?

Did you get feedback on this @springerm ?

River can you confirm you don't have any other issues with this?

Did you get feedback on this @springerm ?

I haven't heard back on this change for the last 2 weeks, so I think he's not following this change anymore. But he approved D143380 (which adds stuff to the new RewriterBase::Listener class), so I think this is fine.

I believe this causes some errors in certain flang builds and is causing one buildbot CI to fail. https://lab.llvm.org/buildbot/#/builders/160/builds/16365

../llvm-project/flang/include/flang/Optimizer/Builder/FIRBuilder.h: In copy constructor ‘fir::FirOpBuilder::FirOpBuilder(const fir::FirOpBuilder&)’:
../llvm-project/flang/include/flang/Optimizer/Builder/FIRBuilder.h:52:3: error: base class ‘struct mlir::OpBuilder::Listener’ should be explicitly initialized in the copy constructor [-Werror=extra]
   52 |   FirOpBuilder(const FirOpBuilder &other)
      |   ^~~~~~~~~~~~

Specifically that is a GCC bot, should you need to reproduce it. Clang is ok with it.

River can you confirm you don't have any other issues with this?

Did you get feedback on this @springerm ?

I haven't heard back on this change for the last 2 weeks,

I know he was out sick..

so I think he's not following this change anymore.

Please get explicit acknowledgement in general and don’t just assume please.

I am out, but this seems alright to me.