Ensure NI_Vector128_AsVector128 (aka Vector128<T> AsVector128(this Vector<T> value)) doesn't have a side-effect in its assert
#76460
Conversation
…ector<T> value)`) doesn't have a side-effect in its assert
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
ghost
assigned 
|
CC. @BruceForstall, @dotnet/jit-contrib |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsThis resolves #76347
|
|
This bug also exists in .NET 6 |
|
(btw, this contributes to #76347, but doesn't fully resolve it. I have other changes to the test running infrastructure that will do the rest of the work) |
|
@tannergooding Do you think this fix should be ported back to .NET 7? .NET 6? What kind of bad codegen implications are there with the unfixed code? Presumably Does a complete fix also require some (or all) of #76456? |
No. That's independent and just helps ensure codegen is better it shouldn't have any impact on the correctness of the codegen.
Its probably better to take it than not given its a 1 line fix putting it back inline with .NET Core 3.1 and .NET 5. That being said, I don't think users will see any negative side effects from us doing nothing... The impact here is that on a system with AVX2 support (which I'd presume is a majority) a user calling vector.AsVector128 will get treated as a "nop" and the value will be consumed directly. Whether that's coming from memory or a register should be fine since the memory slot will be 32-bytes or we'll preserve the 32-bit register. The potential negative would be if CSE or another optimization tries to look at this local and sees TYP_SIMD32 when it should see TYP_SIMD16. However, I'm not aware of any code that would be directly impacted by that... |
|
The superpmi-replay failure looks like a timeout. We've had a few of those lately and need to investigate (separately). |
|
@BruceForstall, should I go ahead and backport this to 7.0 and go through the approval process? What about 6.0? @SingleAccretion or @EgorBo might have a better idea if there is potential CSE impact here. |
|
I would suggest trying to get it in 7.0, since it is silent bad codegen. However, your description of the impact indicates there really is no impact, so that might not persuade the approvers. |
|
One argument for porting it is it will (help) clean up our checked/release asm diffs testing on 7.0, so it could be argued it is a benefit to testing. |
|
/backport to release/7.0 |
|
Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3175903125 |
This resolves #76347