This is an archive of the discontinued LLVM Phabricator instance.

[mlir][irdl] Add `irdl.attributes` operation for defining named attributes
ClosedPublic

Authored by unterumarmung on Jun 10 2023, 4:00 AM.

Details

Summary

This commit introduces the irdl.attributes operation, which allows defining named attributes for the parent operation. Each attribute is defined with a name and a type constraint.

Example usage:

irdl.dialect @example {
  irdl.operation @attr_op {
    %0 = irdl.any
    %1 = irdl.is i64
    irdl.attributes {
      "attr1" = %0,
      "attr2" = %1
    }
  }
}

In this example the operation will expect an arbitrary attribute "attr1"
and an attribute "attr2" with value i64.

Diff Detail

Event Timeline

unterumarmung created this revision.Jun 10 2023, 4:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2023, 4:00 AM
unterumarmung requested review of this revision.Jun 10 2023, 4:00 AM

Thanks a lot for the patch!
I added some inline comments.

I think that one confusion is that constraints should be applied directly on the attributes, and should not constraint the attributes type.
For instance, irdl.attributes { attr1 = %x } should constraint attr1 to the %x constraint, and should not constraint attr1 type to %x.
The reasoning is that we might want to write constraints such as attr1 is AnyOf<"foo", "bar"> to constraint directly the attribute itself,
or something like attr1 is IntegerAttr<%param1, %param2> to constraint it to be an IntegerAttr with the given parameters.
Note that I'm working on making these constraints expressible, for now we can only state the AnyOf one!

mlir/lib/Dialect/IRDL/IRDLLoading.cpp
78

I would get op->getAttrDictionary here, since it is already constructed, and is always sorted.

80–85

In MLIR, it is valid to have extra attributes on operations, so this part of the code should be removed.

89

If you use the dictionary, you can just write getNamed(attrDefName) here.

116–126

I would merge this part with the previous one, so in the end you only iterate to the list of attributes once.

122

This should just be attr.getValue() instead of TypeAttr::get(type).
This is because of the attribute is a TypeAttr, this will stay the same, and if the attribute is another Attribute, this version would fail.

mlir/test/Dialect/IRDL/testd.irdl.mlir
118–121

Here, you are actually constraining two attributes to be exactly i32 and i64.
However I think in your test you want to express that these are IntegerAttr with type i32 and i64.

The reason why we don't want to constraint only the type directly here, is that later on we would like to express things like
AnyOf("foo", "bar") to express that an attribute is either the "foo" string literal or the "bar" literal.

For expressing that we want to constraint the type of an IntegerAttr, we will need more feature in IRDL first :/

So here, I would just keep the same test, but test it with i32 and i64 instead of the IntegerAttr.

Oh also, could you write a verifier in the operation that attributeValues and attributeValueNames have the same length?

unterumarmung added inline comments.Jun 11 2023, 11:43 AM
mlir/lib/Dialect/IRDL/IRDLLoading.cpp
116–126

I haven't merged this part with the previous one because the function consists of two logical parts: (1) Basic error checking and (2) Constraint verification. It seemed appropriate to keep them separate. Additionally, the constraint verification part appears to be more resource-intensive than the basic check. Therefore, I believe it would be more prudent to ensure that all necessary attributes are passed before performing the constraint checking.

Regarding the efficiency of the basic error checking part, I think we can potentially improve its time complexity. Currently, it has a time complexity of O(n*m) and a space complexity of O(1), where n and m represent the sizes of the actual attributes and declared ones, respectively. By storing both arrays in two sets and calculating their intersection using std::set_intersection, we could reduce the time complexity to O(m+n) and the space complexity to O(m+n). However, I understand that this approach may introduce some code complexity, so I refactored it into a separate function.

Nevertheless, I am more than willing to implement it according to your preferences. I just wanted to provide my reasoning and suggest a possible solution to enhance its performance.

122

If I do so, I get the following error

within split at mlir/test/Dialect/IRDL/testd.mlir:238 offset :5:3: error: unexpected error: expected 'i32' but got '42 : i32'
  "testd.attrs"() {attr1 = 42 : i32, attr2 = 42 : i32} : () -> ()
  ^

So, I guess to make it work I need to fix verifiers, right?
Could you give any pointer on where to look at or how to fix it the best way, please?

Mogball added inline comments.Jun 11 2023, 4:36 PM
mlir/include/mlir/Dialect/IRDL/IR/IRDLOps.td
290

please fit this into lines less than 80 characters wide

mlir/lib/Dialect/IRDL/IR/IRDL.cpp
79

please drop the 4. The practice is to not use explicit sizing unless there is a compelling reason to.

84–85

?

mlir/lib/Dialect/IRDL/IRDLLoading.cpp
78

please spell out this auto

80

this one too. The rule is that auto should only be used if the type is spelled verbatim on the RHS like auto thing = dyn_cast<ThingOp>(op) or "too complicated" like DenseMap<int, int>::iterator

87

same here

90

llvm::find_if

92

why not pointer compare the StringAttrs? Instead of the StringRefs

120
math-fehr added inline comments.Jun 12 2023, 4:18 AM
mlir/lib/Dialect/IRDL/IRDLLoading.cpp
116–126

I see!

So one thing to note is that it is fine if the cost is a bit bigger when the verifier fail. After all, this happens at most once, since it will fail the entire compilation pipeline. So merging both will make it faster for the cases that matter, and only a bit slower on the others.

Then, if we want to actually optimize more the performance, we can actually rely on the fact that DictionaryAttr is sorted, so we can just iterate on both the dictionary and the constraints once, if we sort the constraints beforehand. That way we get O(m+n) time and still O(1) size.

122

Yes, you would need to change the tests as well.
The idea is that in your current code, you are constraining the attribute to be a typed attribute of type i32.
However, in our verifiers, we want to directly constraint the attribute itself, not its type.

We don't have examples here yet of that, since we only constraint types for now, but the idea is that you just call verifier.verify({emitError}, attr, constraint) instead of extracting a Type subelement and then passing it to a TypeAttr.

unterumarmung added inline comments.Jun 12 2023, 1:57 PM
mlir/lib/Dialect/IRDL/IRDLLoading.cpp
122

So, if IRDL doesn't support constraining anything but values, how should we test that it can constraint values also?
Or should I just remove the test with the constraint verification for now?
Or maybe there is a possible fix in the ConstraintVerifier that just searches for the type if it sees a type constraint?

unterumarmung added inline comments.Jun 12 2023, 2:11 PM
mlir/lib/Dialect/IRDL/IRDLLoading.cpp
92

I'm not very familiar with MLIR in general. Comparing 2 unrelated (at least, for the MLIR engine) StringAttrs by pointers never occured to me.
Is it guaranteed somehow that attributes with the same value are created only once and then reused?

122

anything but values

I meant "anything but types"

math-fehr added inline comments.Jun 13 2023, 3:07 AM
mlir/lib/Dialect/IRDL/IRDLLoading.cpp
92

Yes, whenever a StringAttr is created, it is uniqued by the MLIRContext (a StringAttr is actually just a pointer to the actual value).
So if you have two StringAttr that encodes the same value, you can just compare them by equality, and this will be done by a pointer equality.

122

So the current way in the ConstraintVerifier is currently intended, so in your case you should just test it with i32 and i64 values (so directly "attr1" = i32).

We need to add more support to constraint directly the type of an attribute, but I would need to think about it some more to know what is the best way (I believe for now the best way would be to add external named constraints).

unterumarmung marked 14 inline comments as done.

Updated code according to review

@math-fehr @Mogball Thank you for the review! I've reworked the code accroding to your suggestions and it got quite nicer. Please take a look.


Oh also, could you write a verifier in the operation that attributeValues and attributeValueNames have the same length?

I'm not sure if it's needed because this rule is enforced by the operation parser, isn't it?
But on the other hand I may be missing something. If you think that the verifier is needed, could you please provide a test case that tests this check? I couldn't come up with any.

mlir/lib/Dialect/IRDL/IRDLLoading.cpp
87

The type here llvm::detail::DenseMapPair<mlir::StringAttr, unsigned long>
I'm not sure that it should be spelled out since it's too verbose

Also, while refactoring according the review, I got rid of it

89

That's markedly better, thanks!

90

Thanks, I was looking for llvm STL algorithms but some how missed STLExtras.h header

116–126

If you mean that we can zip them and iterate checking if the pair we got contains the same-named attributes, I'm not sure it's possible because, as you mentioned, MLIR allows extra attributes to be passed, so they would interfere.
If you meant that we can use "two pointers" technique - that is possible, yes. But I'm not sure whether this approach does not reduce the code quality.

I merged the two parts as you suggested and took advantage of the fact that attributes on actual operation are sorted, so the whole algorithm now is O(log(n)*m)
Is this approach acceptable?

unterumarmung retitled this revision from [mlir][irdl] Add irdl.attributes operation for defining named attributes to [mlir][irdl] Add `irdl.attributes` operation for defining named attributes.Jun 14 2023, 1:44 PM
unterumarmung edited the summary of this revision. (Show Details)

Fix formatting

I'm not sure if it's needed because this rule is enforced by the operation parser, isn't it?
But on the other hand I may be missing something. If you think that the verifier is needed, could you please provide a test case that tests this check? I couldn't come up with any.

It's enforced by the operation parser, but only if you use the custom format.
If you use the generic format, or if you create the operation in C++, this is not enforced anymore.
For a test, you can just write it using the generic format (something like "irdl.attributes"() { attributeValueNames = ["name"] }).

Other than this, this looks good to me!

mlir/include/mlir/Dialect/IRDL/IR/IRDLOps.td
287–288
mlir/lib/Dialect/IRDL/IRDLLoading.cpp
116–126

Yes, I meant a "two pointers" algorithm.
But I'm fine with a O(log(n) * m) complexity, as anyway we should have an even lower dialect if we want better performances!

unterumarmung marked 4 inline comments as done.

Rebased and fixed review comments

If you use the generic format, or if you create the operation in C++, this is not enforced anymore.

Yes, you are right! Totally forgot about the generic format.

All review comments are fixed, please take a look.

You should add the check in the verifier rather than in the loading function.
When we create any IRDL program (even if we don't load it), it should trigger an error.
Can you also add a test for that?

mlir/lib/Dialect/IRDL/IRDLLoading.cpp
197–203

This should be added in the operation verifier rather than here.
Also, when we load an IRDL program, we expect the program to verify, so we won't need it there as well.

Mogball accepted this revision.Jun 19 2023, 10:01 AM
Mogball added inline comments.
mlir/include/mlir/Dialect/IRDL/IR/IRDLOps.td
291
292
mlir/lib/Dialect/IRDL/IRDLLoading.cpp
117–119

drop trivial braces

194–195

please spell out these autos

This revision is now accepted and ready to land.Jun 19 2023, 10:01 AM

LGTM but please wait for @math-fehr's approval

unterumarmung marked 4 inline comments as done.

Address review comments

unterumarmung edited the summary of this revision. (Show Details)Jun 19 2023, 11:25 AM

@math-fehr You are right that I should've implemented the verifier method instead of checking the operation validnesses at loading.
I've done so and added a new test file for this purpose because I couldn't find the appropriate one

@Mogball thank you for the approve, I've addressed your comments

math-fehr accepted this revision.Jun 19 2023, 4:58 PM

Perfect! Thanks a lot!