Page MenuHomePhabricator

[mlir] RewriterGen NativeCodeCall matcher with ConstantOp matcher
ClosedPublic

Authored by rsuderman on Oct 9 2020, 1:56 PM.

Details

Summary

Added an underlying matcher for generic constant ops. This
included a rewriter of RewriterGen to make variable use more
clear.

Diff Detail

Event Timeline

rsuderman created this revision.Oct 9 2020, 1:56 PM
Herald added a project: Restricted Project. · View Herald Transcript
rsuderman requested review of this revision.Oct 9 2020, 1:56 PM
rriddle added inline comments.Oct 9 2020, 2:06 PM
mlir/include/mlir/IR/Matchers.h
274 ↗(On Diff #297332)

Is this necessary? Seems like this is just success(m_Constant(&attribute).match(op)) which is something that could be written inline in DRR and not here.

mlir/include/mlir/IR/OpBase.td
2355

I'd prefer that this has something like "Matcher" in the name so that it is obvious this is just for DRR, given that there is a ConstantLike trait.

rsuderman updated this revision to Diff 297337.Oct 9 2020, 2:18 PM

Updated for riverriddle comments.

rsuderman marked 2 inline comments as done.Oct 9 2020, 2:20 PM

I think you may need to rebase this on head, seems like you were working of an old version here.

mlir/lib/TableGen/Pattern.cpp
220

Would we need to here similarly consider the type as above?

535

New error helper functions were added to TableGen, so one can now say PrintFatalError(&def, ...)

564

nested?

569

Maybe phab is confusing me here, but this looks like same then and else bodies.

573

You might see the other change allowing this, where these become constraints to check for equality.

mlir/test/lib/Dialect/Test/TestOps.td
791

You can skip this if empty.

mlir/tools/mlir-tblgen/RewriterGen.cpp
256

os is a raw_indented_ostream here, so you should just need to use a DelimitedScope here [did you check how this looks? I would expect this to have more indenting than expected]

rsuderman updated this revision to Diff 297710.Oct 12 2020, 4:06 PM

Updated for jpienaar@ comments

rsuderman marked 7 inline comments as done.Oct 12 2020, 4:23 PM
rsuderman added inline comments.
mlir/lib/TableGen/Pattern.cpp
220

We would if it were possible :/. NativeCodeCall doesn't specify types so I need to assume the result (I went with attribute). Long term we need a better solution but I couldn't think of one.

569

The else body statement is the same with the exception of what type of binding is being done. Note bindAttr vs bindValue.

573

I think I implemented as you hoped by adding a verification on the constraint as well. Should be good enough anyway, I am not certain value matching is going to work well for the pattern matcher with native code calls.

mlir/tools/mlir-tblgen/RewriterGen.cpp
256

Indenting is good until the match checks (over indented). I tweaked a little more to decrease the indenting however emitMatchCheck has weird indenting mechanics.

rsuderman updated this revision to Diff 297717.Oct 12 2020, 4:36 PM
rsuderman marked 4 inline comments as done.

Rest of jpienaar@ comments

jpienaar added inline comments.Oct 13 2020, 2:34 PM
mlir/lib/TableGen/Pattern.cpp
220

Yes I think we need to add support for this ODS side (I don't think it would be too hard). Could you add a TODO here to use a more exact type if available?

533

Could we do an early exist instead with positive logic?

564

Could you test with ASAN too? (you might actually need to add some failing test cases too to verify here and below, a TD while where these are invoked from the run should enable covering these).

566

Lets be consistent with all these we now add and use the new helper methods.

585

Could flipping logic + using continues etc be used to reduce this nesting?

mlir/tools/mlir-tblgen/RewriterGen.cpp
242

You should not need to be doing these manual indent calculations here. These are structured, so you should simply be able to have the scope handle that for you. I see it is still below in the other one too at head used in one call, I think it could be an oversight there too, but I'm not sure why it should not be needed.

449

These should not be needed. As mentioned in previous comments, this appears like you based this on a old revision.

rsuderman updated this revision to Diff 297999.Oct 13 2020, 5:16 PM

Additional jpienaar changes.

rsuderman marked 6 inline comments as done.Oct 13 2020, 5:19 PM
rsuderman added inline comments.
mlir/lib/TableGen/Pattern.cpp
564

Invoked with ASAN. Added tests look good (excluding already failing tests).

564

ASAN marks things as good. Could you give me a better feeling on what a TD failure case looks like? Most of the tablegen failures are a bit bewildering.

mlir/tools/mlir-tblgen/RewriterGen.cpp
242

Yeah, I missed some of the merge issues when updating. Should be good now.

jpienaar added inline comments.Oct 14 2020, 8:21 AM
mlir/lib/TableGen/Pattern.cpp
564

Sure, adding a test/tests where we hit these fatal error cases. Given it fatals, one would have to combine something like // RUN: not ... 2>&1 | FileCheck (look at llvm/test/TableGen/defvar.td test for a failure testing case approach that could work well here too)

rsuderman marked an inline comment as done.

Added tablegen rewriter error tests.

rsuderman marked 2 inline comments as done.Oct 14 2020, 12:30 PM
jpienaar accepted this revision.Oct 15 2020, 1:18 PM

Looks good thanks!

mlir/lib/TableGen/Pattern.cpp
535

I think you might have missed this one (or uploaded other version)

This revision is now accepted and ready to land.Oct 15 2020, 1:18 PM
rsuderman updated this revision to Diff 298514.Oct 15 2020, 4:32 PM

Handled last jpienaar comments

This revision was landed with ongoing or failed builds.Oct 15 2020, 4:34 PM
This revision was automatically updated to reflect the committed changes.