Page MenuHomePhabricator

[MLIR] Fix default builders generated by TableGen to use TypeRange for result types
ClosedPublic

Authored by jurahul on Sep 18 2020, 11:47 AM.

Details

Summary
  • Change the default builders to use TypeRange instead of ArrayRef<Type>
  • Add ability to selectively skip either the default separate param or collective param builders, and use that in LinalgStructuredOps to skip default separate param ones, as they conflict with the new default one's since they both use TypeRange now.

Diff Detail

Event Timeline

jurahul created this revision.Sep 18 2020, 11:47 AM
jurahul requested review of this revision.Sep 18 2020, 11:47 AM
jpienaar accepted this revision.Sep 21 2020, 10:13 AM

Lets just focus on "Change the default builders to use TypeRange instead of ArrayRef<Type>" part as one change rather than having 2 unrelated changes in one. That part LGTM to, the other I'd prefer a follow up discussion.

Fix to me implies a bug, here this is just updating it to use TypeRange, don't believe there is any bug being addressed here.

mlir/include/mlir/IR/OpBase.td
2020

Do we really need all of these vs just being able to opt out?

Seems unrelated to the rest so I'd split this out either way and we can discuss in the follow up change.

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
1254

Lets keep a change restricted to one change and so just have this change here.

This revision is now accepted and ready to land.Sep 21 2020, 10:13 AM
jurahul added inline comments.Sep 21 2020, 10:38 AM
mlir/include/mlir/IR/OpBase.td
2020

Yes, this was needed to get everything to compile. mlir-linalg-ods-gen.cpp defines custom builders that conflict with the "default separate" ones that ODS now generates because now both use TypeRange as the argument types. Before this change, ODS was using ArrayRef and the ones defined in mlir-linalg-ods-gen.cpp were using TypeRange, so compilation was successful. I did try skipping all default builders in mlir-linalg-ods-gen.cpp but that does not work either, as it looks like the code relies on default collective param builders being there. Hence this change.

We can definitely split this up, but then the ability to skip default separate param builds would need to come first, followed by the TypeRange change. Let me know if that works. Also, I added skipDefaultCollectiveParamBuilders to be complete, but there is no use for it today, so we can skip adding that as well.

jpienaar added inline comments.Sep 21 2020, 2:08 PM
mlir/include/mlir/IR/OpBase.td
2020

I don't think we want to maintain this level of customization wrt build method generations. Why does the makes redundant approach not address this? E.g., the custom builder here overlaps with generated one, shouldn't the user provided one get preference then?

jurahul added a subscriber: ftynse.Sep 21 2020, 5:04 PM
jurahul added inline comments.
mlir/include/mlir/IR/OpBase.td
2020

Because it only works with internally generated build methods (the resolved ones). User generated ones are never checked against default generated ones yet (to avoid C++ parsing). One option is to extend the code to support that. @ftynse (D88012) put up a code review recently that had support for parsing the user supplied parameters for a prototype, may be we could incorporate that and eliminate the possibility of unresolved parameters (and extend redundancy checking to never eliminate a user defined build method).

jurahul updated this revision to Diff 293635.Sep 22 2020, 8:59 PM

Eliminate the need to selectively skip default builders

ftynse added inline comments.Sep 23 2020, 12:36 AM
mlir/include/mlir/IR/OpBase.td
2020

Please tag me on the follow-up diff when it's up for review :)

I am supportive of the idea that user-defined builders should just override autogenerated builders. This sounds brittle though, we will have to rely on a relatively simple parser (I don't think we want a build dependency on clang) to demonstrate type equivalence. This could work as long as the argument types in autogenerated builders are relatively simple. If we get, e.g., an attribute with value type DenseMap<string, UserDefinedType>, which means a value of this type is passed into one of the autogenerated builders, and a user-defined builder that takes a UserDefinedMap instead, the type equivalence will depend on user code not having a visible alias using UserDefinedMap = DenseMap<string, UserDefinedType>. There is also the question of overload resolution, e.g. between Typename and const Typename & that are different types per se but are considered ambiguous by overload resolution if an instance of Typename is passed.

jurahul marked 2 inline comments as done.Sep 23 2020, 9:02 AM
jurahul added inline comments.
mlir/include/mlir/IR/OpBase.td
2020

I've skirted the whole issue for now by following Jacque's suggestion of disabling all default builders for the LinalgStructuredOps and avoiding introducing the mechanism to selectively skip some of the default ones but not all.

I think the more robust approach is to change ODS OpBuilder to specify the builder parameters using DAG style specification (like you suggested) so we do not have to parse a C++ snippet to piece out the parameters. Yes, there is still the issue of seemingly different names causing problems with overload resolution.

This revision was automatically updated to reflect the committed changes.
jurahul marked an inline comment as done.