[CPU] Enable 'iree-llvmcpu-reassociate-fp-reductions' by default #13685
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR is mostly to have a discussion about enabling fp reduction reassociation by default. When this flag is disabled, we are basically not vectorizing the reduction dimension at all, which results in extra unrolling of scalar instructions. I think it's a bit difficult that an external user really understands the implications of this flag and that it has to be enabled to get some performances on fp reductions. In my case, even though I'm aware of it, I always forget to use it and always waste time looking into "why this random sequence ofinstruction is scalar?" in the profiles. Also, in terms of accuracy, I think we are not paying the same attention to other operations that we are approximating by default (e.g., Math ops), so I think enabling this by default and providing a flag to disable it's reasonable. In the future, we may think about introducing a compilation mode that optimizes for accuracy more generally. WDYT?
dcaballe
added
benchmarks:x86_64
|
I am fine with enabling by default. For the other math ops that have similar precision issues, we should guard it by the same flag..... |
|
The tests failures seem reasonable.... I am OK with enabling this flag, but just beware, we might get a lot of precision related bugs, cause we are now changing the precision. |
hanhanW
approved these changes
May 19, 2023
| ) | ||
|
|
||
| iree_check_single_backend_test_suite( | ||
| name = "check_regression_llvm-cpu", |
There was a problem hiding this comment.
Do we exclude the test in this test suite?
|
I did not notice that auto-merged was enabled.. Sorry about that if you intended to land it later.. |
|
Sorry, my fault. I sometimes enable it when I think the review is mostly done or it's a trivial change to minimize the turnaround if the PR is approved. Let me exclude the test. |
|
I'm surprised that we can pass the test. I thought it should fail when I read the file name. EDIT: I'm good with current state, I'm just surprised. |
|
The test would fail but it's not part of the |
hanhanW
pushed a commit
to hanhanW/iree
that referenced
this pull request
May 26, 2023
…e-org#13685) This PR enables fp reduction reassociation by default. When this flag is disabled, we are basically not vectorizing the reduction dimension at all, which results in extra unrolling of scalar instructions. It's difficult that an external user really understands the implications of this flag and that it has to be enabled to get some performance on fp reductions.
hanhanW
pushed a commit
to hanhanW/iree
that referenced
this pull request
May 26, 2023
…e-org#13685) This PR enables fp reduction reassociation by default. When this flag is disabled, we are basically not vectorizing the reduction dimension at all, which results in extra unrolling of scalar instructions. It's difficult that an external user really understands the implications of this flag and that it has to be enabled to get some performance on fp reductions.
NatashaKnk
pushed a commit
to NatashaKnk/iree
that referenced
this pull request
Jul 6, 2023
…e-org#13685) This PR enables fp reduction reassociation by default. When this flag is disabled, we are basically not vectorizing the reduction dimension at all, which results in extra unrolling of scalar instructions. It's difficult that an external user really understands the implications of this flag and that it has to be enabled to get some performance on fp reductions.
NatashaKnk
pushed a commit
to NatashaKnk/iree
that referenced
this pull request
Jul 6, 2023
hanhanW
pushed a commit
to hanhanW/iree
that referenced
this pull request
Jul 18, 2023
…e-org#13685) This PR enables fp reduction reassociation by default. When this flag is disabled, we are basically not vectorizing the reduction dimension at all, which results in extra unrolling of scalar instructions. It's difficult that an external user really understands the implications of this flag and that it has to be enabled to get some performance on fp reductions.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is mostly to have a discussion about enabling fp reduction reassociation by default. When this flag is disabled, we are basically not vectorizing the reduction dimension at all, which results in extra unrolling of scalar instructions. I think it's a bit difficult that an external user really understands the implications of this flag and that it has to be enabled to get some performance on fp reductions. In my case, even though I'm aware of it, I always forget to use it and always waste time looking into "why this random sequence of instruction is scalar?" in the profiles. Also, in terms of accuracy, I think we are not paying the same attention to other operations that we are approximating by default (e.g., Math ops), so I think enabling this by default and providing a flag to disable it's reasonable. In the future, we may think about introducing a compilation mode that optimizes for accuracy more generally. WDYT?