-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
JIT: Implied branch is not removed #111725
Comments
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Smaller repro: public static void Test(uint x)
{
if (x < 16)
return;
if (x < 8)
Console.WriteLine(); // should be eliminated as never reachable
} ; Method Benchmarks:Test(uint) (FullOpts)
G_M52469_IG01:
sub rsp, 40
G_M52469_IG02:
cmp ecx, 16
jb SHORT G_M52469_IG04
G_M52469_IG03:
cmp ecx, 8
jae SHORT G_M52469_IG04
call [System.Console:WriteLine()]
G_M52469_IG04:
nop
G_M52469_IG05:
add rsp, 40
ret
; Total bytes of code: 26 |
The logic in // Check whether the dominating compare being "false" implies the dominated compare is known
// to be either "true" or "false".
RelopResult treeOperStatus = IsCmp2ImpliedByCmp1(GenTree::ReverseRelop(domOper), domCns, treeOper, treeCns);
if (treeOperStatus == RelopResult::Unknown)
{
return false;
|
Looks like this is not too hard to add. I had to relax an assert because now we have cases where the dominated relop is fully redundant ( if (x < 47)
{
if (x <= 46) which we didn't see from other forms of predicate inference. (arguably, perhaps, VN should be equating these, eg normalizing all LTs to LEs, at least for cases involving constants where we can avoid getting into trouble with overflow). |
Thanks to dotnet#95234, RBO can draw inferences when the same value is compared to different constants, if the initial comparison dominates and was false. Generalize this to also handle cases where the initial comparison dominates and is true. Fixes dotnet#111725.
Here two functions do the same thing but their codegen is different:
cc @AndyAyersMS @dotnet/jit-contrib
The text was updated successfully, but these errors were encountered: