JIT: Don't assume unsigned division always produces non-negative result in value numbering#109384
Conversation
ghost
added
the
area-CodeGen-coreclr
| case VNF_EQ: | ||
| case VNF_NE: | ||
| case VNF_UMOD: | ||
| case VNF_UDIV: |
There was a problem hiding this comment.
Is this the "right" thing? The result remains unsigned, it just has the most significant bit set.
Does IsVNNeverNegative actually behavior more like IsMsbNeverSet and should a comment be added?
It isn't clear why UDIV is problematic but VNF_ADD_UN_OVF is not, considering that say Int.MaxValue + 1 would produce 0x8000_0000 (a positive unsigned value, but equivalent to int.MinValue if interpreted as a signed value).
The same question would also apply to UMOD where 2147483648u % 2147483649u would still produce 2147483648u (0x8000_0000)
There was a problem hiding this comment.
Indeed IsVNNeverNegative should be taken to mean whether the value is non-negative when considered as a signed value. Since signedness/unsignedness is a property of operations in the JIT it doesn't really make sense to talk about signed/unsigned values.
I agree it looks like more operations need to be removed from this list.
There was a problem hiding this comment.
The same question would also apply to UMOD where 2147483648u % 2147483649u would still produce 2147483648u (0x8000_0000)
I can readily repro bad codegen for this scenario. I can't get the right code shape for the arithmetic overflow scenarios to produce bad codegen, though based on your explanation, I agree they should be removed, too.
There was a problem hiding this comment.
Indeed IsVNNeverNegative should be taken to mean whether the value is non-negative when considered as a signed value. Since signedness/unsignedness is a property of operations in the JIT it doesn't really make sense to talk about signed/unsigned values.
👍
This seems like something that is very easy to get wrong given the current naming of the flag/query. It might be beneficial to consider whether a better name exists as part of a follow up.
There was a problem hiding this comment.
This seems like something that is very easy to get wrong given the current naming of the flag/query. It might be beneficial to consider whether a better name exists as part of a follow up.
The comment I added reaffirms your suggestion of IsMsbNeverSet -- I can switch over to that now, since we're here.
There was a problem hiding this comment.
There's a number of other places using the "NeverNegative" terminology. If we rename one we should rename all of them, otherwise I think it's better to be consistent.
There was a problem hiding this comment.
Agree, IsMsbNeverSet doesn't sound great/easy to discover. I think a more generic solution is to return "bits used" with additional helpers on top (that's how it's done in LLVM), till that is implement I propose we stick to IsVNNeverNegative.
There was a problem hiding this comment.
Got it, reverted.
There was a problem hiding this comment.
If we rename one we should rename all of them
IsMsbNeverSet doesn't sound great/easy to discover.
Agreed on both points. That's why I had suggested considering other names as a follow up. Something like ISVNNeverNegativeWhenSigned might be clearer and just makes the existing name more verbose, for example.
I'm sure between all of use we can get some good suggestions and come up with a decent name
|
Diffs show small regressions as expected. |
4 participants
Fixes #109337. This pessimized relatively few methods locally, and the underlying optimization was expanded with #105593 in .NET 10, so this churn seems tolerable.
cc @dotnet/jit-contrib, @jakobbotsch PTAL. Thanks!