Page MenuHomePhabricator

[mlir][assemblyFormat] Fix bug when using AttrSizedOperandSegments trait with only non-buildable operand types
ClosedPublic

Authored by maerhart on Apr 28 2020, 6:50 AM.

Details

Summary

When creating an operation with

  • AttrSizedOperandSegments trait
  • Variadic operands of only non-buildable types
  • assemblyFormat to automatically generate the parser

the builder local variable is used, but never declared.
This adds a fix as well as a test for this case as existing ones use buildable types only.

Diff Detail

Event Timeline

maerhart created this revision.Apr 28 2020, 6:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2020, 6:50 AM
Kayjukh added inline comments.Apr 28 2020, 7:34 AM
mlir/tools/mlir-tblgen/OpFormatGen.cpp
743–744

This could be removed and the next lines adapted as noted in the comment.

745

We should follow what is done in OperationFormat::genParser and use parser.getBuilder() instead of a potentially fragile local variable declaration.

868

This seems to be a somewhat fragile fix, especially regarding the mirrored declaration of Builder &builder in OperationFormat::genParserTypeResolution. To reduce the coupling between the functions in this file, I would rather suggest to replace the usages of builder with parser.getBuilder().

maerhart updated this revision to Diff 260639.Apr 28 2020, 7:59 AM

Updated the diff to use parser.getBuilder() directly instead of creating a fragile local variable.

Kayjukh added a comment.EditedApr 28 2020, 8:08 AM

nit: The commit message looks a little convoluted. Maybe you could simplify/structure it a little bit.
Otherwise, LGTM.

maerhart edited the summary of this revision. (Show Details)Apr 28 2020, 8:42 AM
Kayjukh accepted this revision.Apr 28 2020, 8:46 AM
This revision is now accepted and ready to land.Apr 28 2020, 8:46 AM
This revision was automatically updated to reflect the committed changes.

Thanks for the fix!