Skip to content

Conversation

@jtuyls
Copy link
Contributor

@jtuyls jtuyls commented Oct 27, 2025

This was causing an assertion failure in llama 8b fp8 reported by @pdhirajkumarprasad. I will need to fix this as well in upstream and will add a testcase there. Then this will be removed here once upstream is integrated.

@pdhirajkumarprasad
Copy link

I tried original design with the above change and it's working fine

Copy link
Collaborator

@benvanik benvanik left a comment

Choose a reason for hiding this comment

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

You need a test.

(I'm not liking that a memref thing is in the Util dialect specific file - if you're planning to remove this that's good, but in the future don't do that please)

@jtuyls
Copy link
Contributor Author

jtuyls commented Oct 27, 2025

You need a test.

(I'm not liking that a memref thing is in the Util dialect specific file - if you're planning to remove this that's good, but in the future don't do that please)

@benvanik This is not that easy to test in IREE itself because we don't include test.reify_bounds. I think it belongs in upstream, as mentioned in the original PR and in the description here as well and it will be removed soon. Here is the upstream PR that fixes it with test: llvm/llvm-project#165333.

Edit: the upstream PR got merged already.

(I'm not liking that a memref thing is in the Util dialect specific file - if you're planning to remove this that's good, but in the future don't do that please)

This is not in a util dialect specific file, it contains all kinds of interface implementations for upstream operations.

@jtuyls
Copy link
Contributor Author

jtuyls commented Oct 27, 2025

@benvanik Also, considering this is a hotfix for our e2e shark tests and benchmarks and it will be tested in upstream and removed here very soon, could we just land this as is?

Copy link
Collaborator

@MaheshRavishankar MaheshRavishankar left a comment

Choose a reason for hiding this comment

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

This looks fine to me if this is going to be landed upstream..
Or @jtuyls for hot fix like this, just add what you need to the iree fork of LLVM and you can commit this upstream and drop the commit in IREE submodule.

@jtuyls
Copy link
Contributor Author

jtuyls commented Oct 29, 2025

Closing in favor of #22460

@jtuyls jtuyls closed this Oct 29, 2025
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.

5 participants