-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[x86 & HVX & WASM] Use bounds inference for saturating_narrow instruction selection #7805
Conversation
Understood, but is there any other reasonable way to add checks for this? This sort of stuff is often pretty fragile. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I feel like if we don't have some way to test this, we'll eventually find it broken :-/
I thought about adding a "check_not" to simd_op_check, because that's the only way I could think to test this sort of optimization, but that seems like heavy functionality to use for just this case (I'm also not sure we can verify that this optimization runs by saying "don't give me a |
We can do the same thing for HVX saturating narrow instructions, which I was going to do in a separate PR, but it requires the same changes to Bounds.cpp and Type.h that that the x86 version does, so I am making this one PR. |
@steven-johnson Luckily, the improvement to HVX can be tested (unlike x86). Unluckily, this revealed that constant bounds inference does not work on u32 like I expected (which is part of why @abadams opened #7807) |
Is this PR still an active one? It seems pretty stale. |
What's the story, needs more work? |
This has gotten stale, but I am trying to find time to update it. Andrew and I discussed some issues with using bounds inference in this way, but I think they are fixed by using the new infrastructure in #8179. I will aim to do this in the next two weeks, I've finally started to catch up on my insane backlog. |
I think it should be fairly straight-forward to adapt it to use ConstantInterval, now that the ConstantInterval refactor is in. You could do it now and there wouldn't be much of a merge conflict, or you could wait for #8155 Here's how I use ConstantInterval to do something similar: https://github.com/halide/Halide/pull/8155/files#diff-6cd3cc01186f9ce4abaa1a98c9201bd440983ecc5620fbccae383df1116bdce6R704 All your uses are about something being safe to reinterpret as signed/unsigned, so I think you want a few instances of |
Yeah, realized it wasn't a ton of work, and was bored during an ASPLOS workshop. This is good for review now! Also, the u32 HVX tests now work, because the new ConstantInterval stuff is stronger than find_constant_bounds on u32. Nice side effect. |
@abadams Sorry to add a change after your approval, but I just saw a wasm talk and was reminded that we can do this for wasm too. It's basically a copy-paste from x86, but would be nice to have eyes just to be safe. |
The big TODO for wasm now is exploiting constant_integer_bounds to hit the relaxed simd instructions in case where we know they don't have UB |
Yep, I think #7312 is for tracking that. This is still a nice side benefit of constant bounds for wasm though |
I don't think relaxed SIMD has actually shipped in wasm yet, so that's still waiting on the wasm folks |
https://webassembly.org/features/ TL;DR: shipped in Chrome, behind flags elsewhere |
There are cases where bounds inference allows us to safely reinterpret unsigned values as signed values, in order to target x86 saturating narrow instructions. I give an example below. I don't add tests to simd_op_check_x86 because we can't test for removing
min
instructions via the existing mechanism.Example:
Before:
After (removed 2
vpminuw
instructions):