This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Make ConversionTarget dynamic legality callbacks composable
ClosedPublic

Authored by Hardcode84 on Sep 25 2021, 11:59 AM.

Details

Summary
  • Change callback signature bool(Operation *) -> Optional<bool>(Operation *)
  • addDynamicallyLegalOp add callback to the chain
  • If callback returned empty Optional next callback in chain will be called

Diff Detail

Event Timeline

Hardcode84 created this revision.Sep 25 2021, 11:59 AM
Hardcode84 requested review of this revision.Sep 25 2021, 11:59 AM

Old discourse thread https://llvm.discourse.group/t/markunknownopdynamicallylegal-is-not-composable/3800

Initially I wanted to do this only for markUnknownOpDynamicallyLegal, but now I have usecase for addDynamicallyLegalOp too:
I have getitem op in my dialect which can be applied to arrays or tuples with different set of conversions and legality setup, and I want to setup them from different places.

Remove debug print

fix clang-tidy warnings

rriddle added inline comments.Sep 30 2021, 5:01 PM
mlir/include/mlir/Transforms/DialectConversion.h
664

I would consider Optional<bool> to be a bit of anti-pattern. Can we switch to an enum/Optional<enum> instead?

mlir/lib/Transforms/Utils/DialectConversion.cpp
2728–2732
2745

nit: Spell out auto here.

Hardcode84 added inline comments.Sep 30 2021, 5:44 PM
mlir/include/mlir/Transforms/DialectConversion.h
664

I think Optional<smth> approach is in line with other APIs (e.g. TypeConverter) and Optional<bool> allow to use existing bool callbacks as is

rebase, address some review comments, more tests

Hardcode84 marked 2 inline comments as done.Oct 6 2021, 12:41 PM
rriddle accepted this revision.Oct 11 2021, 6:38 PM

I am still iffy about the Optional<bool>, but am fine with it for now for consistency sake.

mlir/lib/Transforms/Utils/DialectConversion.cpp
2712–2714

nit: Spell out auto here, and also merge the variable into the if.

mlir/unittests/Transforms/DialectConversion.cpp
19–24

nit: Pull the function out of the anonymous namespace.

This revision is now accepted and ready to land.Oct 11 2021, 6:38 PM