This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Change ODS collective params build method to provide an empty default value for named attributes
ClosedPublic

Authored by jurahul on Jul 9 2020, 3:37 PM.

Details

Summary
  • Provide default value for ArrayRef<NamedAttribute> attributes parameter of the collective params build method.
  • Change the genSeparateArgParamBuilder function to not generate build methods that may be ambiguous with the new collective params build method.
  • This change should help eliminate passing empty NamedAttribue ArrayRef when the collective params build method is used

Diff Detail

Event Timeline

jurahul created this revision.Jul 9 2020, 3:37 PM

Could you add some unit tests?

I looked into extending the op-decl.td unit test, but the way the tests are currently setup we have a mix of mlir and td tests under test/mlir-tblgen, so its not possible to observe the unit test failure because mlir-opt fails to build with a broken mlir-tblgen. In an attempt to make the mlir-tblgen dependent tests to be run independently from mlit-opt dependent tests, I have put up another review (https://reviews.llvm.org/D83528) to split the tests under mlir-tblgen into two directories. I'll update the this one with a unit tests once that change goes in and I can test (easily) that my unit tests fails if I disable the suppression of generating the ambiguous build methods

jurahul updated this revision to Diff 277144.Jul 10 2020, 2:13 PM

Add unit test

So the goal of this change is to avoid needing to specify empty array of attributes if there are none for the collective case? (We have default attributes and so the description was a bit ambiguous wrt that)

so its not possible to observe the unit test failure because mlir-opt fails to build with a broken mlir-tblgen.

Yes, but these unit tests don't test if mlir-tblgen is broken, they test if it produces expected results.

Now if mlir-tblgen is incorrect to the point where mlir-tblgen produces invalid C++ code so that mlir-opt can't compile, then yes we won't see the unit tests mlir-tblgen running. That's a pain and one affecting folks working on mlir-tblgen. Today they could run the test commands manually or just disable also building mlir-opt in the test suite and run the tests, buts it not automatic.

So if we look to improve that, what would the goal be to make it easier for folks developing mlir-tblgen? Or to test valid C++ code is generated? E.g., we don't test the latter except with mlir-opt today and so is the plan to address that? That is also a different discussion and we could discuss on the splitting change to

So the goal of this change is to avoid needing to specify empty array of attributes if there are none for the collective case? (We have default attributes and so the description was a bit ambiguous wrt that)

Yes, that's the goal. Currently I see several uses of this build/create method where we are passing empty array of attributes and the goal is to eliminate that from the code. This has nothing to do with default *values* attributes, I think I saw another bug regarding them, bit this change is not attempting to fix that. I can reword this to "Change collective params build method to have an empty default value for attributes".

As for the unit test issue, yes, lets have that as a separate discussion. For now, am maintaining the status quo there and extending the existing test with addition tests. If/when we migrate the mlir-tblgen unit test to the C++ unittest, I expect these will migrate with that.

jurahul retitled this revision from [MLIR] Add default value for ODS collective params builder `attributes` parameter to [MLIR] Change ODS collective params build method to provide an empty default value for named attributes.Jul 13 2020, 9:27 AM
mehdi_amini added inline comments.Jul 13 2020, 9:36 AM
mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
958

Typo: "collective pramas"

972

Typo: ambiguious -> ambiguous (and same typo below)

(thanks for writing a good doc!)

jurahul updated this revision to Diff 277467.Jul 13 2020, 9:49 AM

Fix spelling in comment

Nice, thanks for expanding the documentation there

mlir/test/mlir-tblgen/op-decl.td
186

suppression ?

Could you expand to say why?

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

Missing )

970

Why is a single variadic result required? I think you might mean at most one variadic result

1221

s/if/as/ ? (if here makes me think there are cases where we get here where this is not true)

jurahul updated this revision to Diff 277484.Jul 13 2020, 10:16 AM

More spellcheck..

jurahul updated this revision to Diff 277488.Jul 13 2020, 10:49 AM
jurahul marked 5 inline comments as done.

Address review comments

jpienaar accepted this revision.Jul 13 2020, 11:02 AM

Nice, thanks

This revision is now accepted and ready to land.Jul 13 2020, 11:02 AM
rriddle added inline comments.Jul 13 2020, 11:03 AM
mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
1221

nit: Please move the comment out of the else body, or add braces. This makes the body "non-trivial".

jurahul updated this revision to Diff 277496.Jul 13 2020, 11:08 AM

Update comment to describe the conditions more clearly

jurahul updated this revision to Diff 277498.Jul 13 2020, 11:11 AM

Braces for else case

rriddle added inline comments.Jul 13 2020, 11:13 AM
mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
1221

nit: When adding braces to an else/if, add the braces to all of the if/elses in the chain.

jurahul added inline comments.Jul 13 2020, 11:18 AM
mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
970

I do mean a single variadic result. The build() method we are trying to suppress is one with a single ArrayReg<Type> argument for result types and a single ArrayRef<Value> for operands, and no other parameters after that. That condition for the first one is either a a collective paramKind for result type, or a separate with a single variadic result. If there are no results, I am assuming the separate paramKind will not generate any ArrayRef<Type> argument for a non-result (buildParamList case TypeParamKind::Separate iterates over numResults, which will be 0).

I guess I could make the comment clearer by segregating these 2 criteria and describing the conditions for these 2 differently (conditions for generating a single ArrrayRef<Type> and condition for generating a single ArrayRef<Value> for operands).

jurahul updated this revision to Diff 277500.Jul 13 2020, 11:20 AM

And braces for if

This revision was automatically updated to reflect the committed changes.