Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Python] Ensure attribute_to_var only accesses value when legal. #5612

Merged
merged 1 commit into from
Jul 18, 2023

Conversation

mikeurbach
Copy link
Contributor

This codepath assumes when the input attribute is not an Attribute, it has a value member. This is the case for things like StringAttr, but not for ArrayAttr. This popped up because upstream MLIR has been working to return downcasted concrete Attribute subclasses in more situations, so APIs that previously returned an Attribute might now return an ArrayAttr. A simpler testcase also reveals the issue. I've updated this path to also ensure that a value member exists before using it, and otherwise fall back to the generic code path.

This codepath assumes when the input attribute is not an Attribute, it
has a value member. This is the case for things like StringAttr, but
not for ArrayAttr. This popped up because upstream MLIR has been
working to return downcasted concrete Attribute subclasses in more
situations, so APIs that previously returned an Attribute might now
return an ArrayAttr. A simpler testcase also reveals the issue. I've
updated this path to also ensure that a value member exists before
using it, and otherwise fall back to the generic code path.
@mikeurbach mikeurbach requested a review from teqdruid July 18, 2023 00:13
@mikeurbach
Copy link
Contributor Author

@teqdruid what do you think? It looks like this path was added to avoid going into the chain of try/except probing, but this would never have worked for passing an ArrayAttr directly to attribute_to_var, since ArrayAttr has no value. Now that more APIs return ArrayAttr directly, we're exposing this issue.

Copy link
Contributor

@teqdruid teqdruid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiousity, what does ArrayAttr have instead of value? I would expect it to have some member which returns an iterable of attributes.

@seldridge seldridge merged commit 0fcb904 into main Jul 18, 2023
5 checks passed
@mikeurbach mikeurbach deleted the mikeurbach/python-attribute-check branch July 18, 2023 16:39
@mikeurbach
Copy link
Contributor Author

It defines __iter__, which returns an iterator over the attributes: https://github.com/llvm/llvm-project/blob/05abcc579244b68162b847a6780d27b22bd58f74/mlir/lib/Bindings/Python/IRAttributes.cpp#L324.

You can do things like for attr in myarray.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants