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

Check isIntOrFloat before querying bitwidth #19172

Merged
merged 7 commits into from
Nov 19, 2024

Conversation

IanWood1
Copy link
Contributor

@IanWood1 IanWood1 commented Nov 16, 2024

The element type must be an int or float before using getIntOrFloatBitWidth. This adds a check if the element type is an int or float, otherwise don't adjust innermost tile size. Introduced by commit c80fa3b

1/2 fix for #19167.

@IanWood1 IanWood1 marked this pull request as ready for review November 18, 2024 17:22
@IanWood1 IanWood1 requested a review from lialan November 18, 2024 17:28
Copy link
Contributor

@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.

Copy link
Contributor

@hanhanW hanhanW left a comment

Choose a reason for hiding this comment

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

I'm not aware that there are types other than int/float being the inputs of codegen. Can you elaborate how you discover the case and how does it get to the CPU codegen inputs? Or can you give me an example that can reproduce it at iree-opt/iree-compile level?

I left a similar comment on #19185 (review)

@MaheshRavishankar
Copy link
Contributor

I'm not aware that there are types other than int/float being the inputs of codegen. Can you elaborate how you discover the case and how does it get to the CPU codegen inputs? Or can you give me an example that can reproduce it at iree-opt/iree-compile level?

I left a similar comment on #19185 (review)

We can have complex types for inputs. LLaMa has complex types.

@IanWood1
Copy link
Contributor Author

IanWood1 commented Nov 18, 2024

I'm not aware that there are types other than int/float being the inputs of codegen. Can you elaborate how you discover the case and how does it get to the CPU codegen inputs? Or can you give me an example that can reproduce it at iree-opt/iree-compile level?

I left a similar comment on #19185 (review)

This was popping up with complex types in https://github.com/nod-ai/shark-ai, but there were recent changes to that repo so I don't know where the llama server integration test has been moved. I think https://gist.github.com/IanWood1/ccbcc62dca7a77d00a4ceaecb8fe38a2 will repro with iree-compile --iree-hal-target-backends=llvm-cpu

Edit: you have to checkout #19187 since the other error is hit befor this one

@hanhanW
Copy link
Contributor

hanhanW commented Nov 18, 2024

I think 3e40798 is a better fix, which adds the computation for complex types.

In this case, can you copy below test with CPU variants to one of select_*.test?

https://github.com/iree-org/iree/blob/main/compiler/src/iree/compiler/Codegen/SPIRV/test/config_default_misc.mlir#L3-L42

Copy link
Contributor

@hanhanW hanhanW left a comment

Choose a reason for hiding this comment

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

Thanks, just a couple nits!

@IanWood1
Copy link
Contributor Author

@hanhanW I used @giacs-epic's commit instead and added a test. I just copied the test you mentioned and adjusted the tile sizes. Would you be able to check if it makes sense?

Also, @giacs-epic are you ok with hanhan's suggestion in #19185 (comment) to commit this with you as coauthor?

Signed-off-by: Ian Wood <[email protected]>
Signed-off-by: Ian Wood <[email protected]>
@lialan
Copy link
Contributor

lialan commented Nov 18, 2024

I'm not aware that there are types other than int/float being the inputs of codegen. Can you elaborate how you discover the case and how does it get to the CPU codegen inputs? Or can you give me an example that can reproduce it at iree-opt/iree-compile level?

I left a similar comment on #19185 (review)

@IanWood1 @hanhanW A few other places that computes size (mostly in emulating narrow types) do not assume the type is a ComplexType. So perhaps we can have a better API call compared to getIntOrFloatBitwidth to get the size of a type? This is a bit confusing.

I will see if EmulatingNarrowTypes needs to support complex types.

@hanhanW
Copy link
Contributor

hanhanW commented Nov 18, 2024

@IanWood1 @hanhanW A few other places that computes size (mostly in emulating narrow types) do not assume the type is a ComplexType. So perhaps we can have a better API call compared to getIntOrFloatBitwidth to get the size of a type? This is a bit confusing.

I will see if EmulatingNarrowTypes needs to support complex types.

We can skip complex types in the emulation for now. My guess is that it is not needed by the emulation. If we need it, then we implement it.

@IanWood1 IanWood1 merged commit b68c535 into iree-org:main Nov 19, 2024
36 checks passed
@IanWood1 IanWood1 deleted the fix_complex_assert branch November 19, 2024 03:50
Groverkss pushed a commit to Groverkss/iree that referenced this pull request Dec 1, 2024
The element type must be an int or float before using
`getIntOrFloatBitWidth`. This adds a check if the element type is an int
or float, otherwise don't adjust innermost tile size. Introduced by
commit
iree-org@c80fa3b

1/2 fix for iree-org#19167.

---------

Signed-off-by: Ian Wood <[email protected]>
Co-authored-by: giacs-epic <[email protected]>
giacs-epic added a commit to giacs-epic/iree that referenced this pull request Dec 4, 2024
The element type must be an int or float before using
`getIntOrFloatBitWidth`. This adds a check if the element type is an int
or float, otherwise don't adjust innermost tile size. Introduced by
commit
iree-org@c80fa3b

1/2 fix for iree-org#19167.

---------

Signed-off-by: Ian Wood <[email protected]>
Co-authored-by: giacs-epic <[email protected]>
Signed-off-by: Giacomo Serafini <[email protected]>
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