[Relax] Migrate NN conv/pooling/grad attrs from Array<IntImm> to Array<int64_t>#18733
[Relax] Migrate NN conv/pooling/grad attrs from Array<IntImm> to Array<int64_t>#18733guan404ming wants to merge 2 commits intoapache:mainfrom
Conversation
Summary of ChangesHello @guan404ming, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of has_attr pattern matching failing for integer attributes with mismatched dtypes. The introduction of the AttrValueEqual helper function in dataflow_matcher.cc is a clean solution. It correctly falls back to value-based comparison for integers and recursively handles arrays, which is necessary for attributes like strides. The changes to MatchAttrs to use this new helper are appropriate. The re-enabling of the test case in test_dataflow_pattern.py confirms the fix. The updated docstring for AttrPattern in python/tvm/relax/dpl/pattern.py accurately reflects its expanded capabilities. Overall, the changes are well-implemented and improve the robustness of the dataflow pattern matcher.
|
Thanks for the contribution. I think this is due to one of the legacy issues. Background: previously we used IntImm for constant attribute, and Integer redirects to IntImm. POD types like |
2c1729b to
bb93d9a
Compare
019b500 to
1421f0b
Compare
Thanks for the suggestion. I adjust the pr to migrate NN conv/pooling/grad attrs from Array to Array<int64_t> which I think to address the issue from real root cause. |
1421f0b to
c3b28dd
Compare
Signed-off-by: Guan-Ming Chiu <guanmingchiu@gmail.com>
c3b28dd to
4b245c1
Compare
Why
has_attr pattern matching failed for integer attributes because StructuralEqual requires exact dtype match for IntImm,
causing mismatches between Python-specified pattern values and actual operator attributes.
How