Page MenuHomePhabricator

[MLIR] Replace dialect registration hooks with dialect handle
ClosedPublic

Authored by GeorgeLyon on Feb 7 2021, 5:12 PM.

Details

Summary

Replace MlirDialectRegistrationHooks with MlirDialectHandle, which under-the-hood is an opaque pointer to MlirDialectRegistrationHooks. Then we expose the functionality previously directly on MlirDialectRegistrationHooks, as functions which take the opaque MlirDialectHandle struct. This makes the actual structure of the registration hooks an implementation detail, and happens to avoid this issue: https://llvm.discourse.group/t/strange-swift-issues-with-dialect-registration-hooks/2759/3

Diff Detail

Event Timeline

GeorgeLyon created this revision.Feb 7 2021, 5:12 PM
GeorgeLyon requested review of this revision.Feb 7 2021, 5:12 PM
GeorgeLyon abandoned this revision.Feb 7 2021, 5:13 PM
GeorgeLyon reclaimed this revision.Feb 7 2021, 5:20 PM
GeorgeLyon retitled this revision from Replace hooks with dialect handle to [MLIR] Replace dialect registration hooks with dialect handle.Feb 7 2021, 5:31 PM
GeorgeLyon edited the summary of this revision. (Show Details)
GeorgeLyon added inline comments.Feb 8 2021, 9:38 AM
mlir/include/mlir-c/Registration.h
41

We can also just make this a symbol instead of a function.

stellaraccident accepted this revision.Feb 8 2021, 10:03 PM
stellaraccident added inline comments.
mlir/lib/CAPI/IR/DialectHandle.cpp
12

Can we match naming used elsewhere, even if the mechanism is different: "unwrapHooks". Have a look at Support.h too: I suspect that you can just do unwrap(handle) if you've defined the hooks struct properly.

29

Newline please.

This revision is now accepted and ready to land.Feb 8 2021, 10:03 PM

Address feedback

GeorgeLyon added inline comments.Feb 8 2021, 10:10 PM
mlir/lib/CAPI/IR/DialectHandle.cpp
12

I'm not an expert in C++, but I'm not sure this would work as MlirDialectRegistrationHooks is a C type. I have changed the name instead.

GeorgeLyon added inline comments.Feb 8 2021, 10:26 PM
mlir/lib/CAPI/IR/DialectHandle.cpp
12

Nevermind, I thought you meant something different. Just changing the name to unwrap works.

Slight tewak