JIT: Remove double-negation in during morph phase #27779
Conversation
damageboy
commented
Nov 8, 2019
damageboy
commented
- fixes #27442
- Co-authored with @EgorBo holding my hand :)
|
I think you still need to check the overflow flag, the following case should throw an exception, shouldn't it? checked
{
int a = int.MaxValue;
Console.WriteLine(a == -(-a));
int b = int.MinValue;
Console.WriteLine(b == -(-b));
}(should work for TYP_FLOAT/DOUBLE without it) |
|
/cc @dotnet/jit-contrib |
There's no overflow flag on |
damageboy
force-pushed
the
wip/issue27442
branch
3 times, most recently
from
b29fb23 to
37077b3
Compare
|
@EgorBo The C# snippet in your comment will result in |
damageboy
force-pushed
the
wip/issue27442
branch
from
37077b3 to
3d8b9af
Compare
sandreenko
left a comment
sandreenko
left a comment
There was a problem hiding this comment.
The change looks good with 1 nit that is optional, thank you, @damageboy.
Could you please collect jit asm diffs so we now how many methods are affected and make sure there are no unexpected regressions?
In order to do that refer to https://github.com/dotnet/jitutils/blob/master/doc/diffs.md, the final command will look like:
C:\git\tools\jitutils\bin\jit-diff.bat diff -b "F:\git\coreclr\bin\tests\Windows_NT.x64.Checked\Tests\Core_Root_base" -d "F:\git\coreclr\bin\tests\Windows_NT.x64.Checked\Tests\Core_Root_diff" -o F:\diffs --core_root F:\git\coreclr\bin\tests\Windows_NT.x64.Checked\Tests\Core_Root -c
src/jit/morph.cpp
Outdated
| case GT_NOT: | ||
| case GT_NEG: | ||
| // Remove double negation | ||
| if (oper == GT_NEG && op1->OperIs(GT_NEG)) |
There was a problem hiding this comment.
Nit: It is strange to see an if on switch operand.
I would do
case GT_NEG:
if (op1->OperIs(GT_NEG))
{
// Remove double negation
GenTree* child = op1->AsOp()->gtGetOp1();
return child;
}
__fallthrough;
case GT_NOT:
old code.
There was a problem hiding this comment.
Sure thing, it wasn't clear to me what the preferred style is, to keep the commonality or separate cases.
damageboy
force-pushed
the
wip/issue27442
branch
3 times, most recently
from
2a6f00c to
e185071
Compare
- fixes #27442 - Co-authored with @EgorBo holding my hand :)
damageboy
force-pushed
the
wip/issue27442
branch
from
e185071 to
8a01d28
Compare
ViktorHofer
added
the
* NO MERGE *
|
Thank you for your contribution. As announced in #27549 the dotnet/runtime repository will be used going forward for changes to this code base. Closing this PR as no more changes will be accepted into master for this repository. If you’d like to continue working on this change please move it to dotnet/runtime. |
|
@damageboy Are you going to open a PR for this in the https://github.com/dotnet/runtime repo? (Or did you already do so and I missed it?) |
|
Yes! I am planning to do this sometime over this weekend :) |
9 participants
