-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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: Substitute constant into loop testing variable where possible #90622
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsAnother try on #90591. Waiting for CI to check the impact. Example: [MethodImpl(MethodImplOptions.NoInlining)]
int Test()
{
var sum = 0;
var config = new Config { Iteration = 4 };
for (var i = 0; i < config.Iteration; i++)
{
sum++;
}
return sum;
}
struct Config
{
public int Iteration;
} Diff for G_M19777_IG01: ;; offset=0x0000
G_M19777_IG02: ;; offset=0x0000
- xor eax, eax
- xor ecx, ecx
- align [0 bytes for IG03]
- G_M19777_IG03: ;; offset=0x0004
+ G_M19777_IG03: ;; offset=0x0005
- inc eax
- inc ecx
- cmp ecx, 4
- jl SHORT G_M19777_IG03
+ mov eax, 4
+ ret
- G_M19777_IG04: ;; offset=0x000D
- ret
|
This comment was marked as outdated.
This comment was marked as outdated.
Testing failure seems unrelated. |
My personal opinion: this does not have enough hits to justify. I would also expect that @AndyAyersMS's planned change to assertion prop during morph will address these cases more naturally by enabling non-local constant propagation. |
ac22da7
to
091c79f
Compare
69ffe00
to
a5e5fd6
Compare
@@ -1870,7 +1870,7 @@ bool Compiler::optIsLoopClonable(unsigned loopInd) | |||
if (requireIterable) | |||
{ | |||
if ((loop.lpFlags & LPFLG_CONST_LIMIT) == 0 && (loop.lpFlags & LPFLG_VAR_LIMIT) == 0 && | |||
(loop.lpFlags & LPFLG_ARRLEN_LIMIT) == 0) | |||
(loop.lpFlags & LPFLG_CONST_VAR_LIMIT) == 0 && (loop.lpFlags & LPFLG_ARRLEN_LIMIT) == 0) |
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.
nit push the (loop.lpFlags & LPFLG_ARRLEN_LIMIT) == 0)
to a newline
Made it a bit more general. Now we have more diffs can be observed. |
Copy prop test
Ping @BruceForstall to review this community PR. |
I think we should not take this PR. It looks like it can find some interesting cases to optimize, but it's not clear how often those occur in practice, and the implementation looks quite expensive to just be feeding loop cloning in this specific case. Also, there is a plan to have a more general assertion propagation occur during morph, which might create a better underpinning for this optimization. See #93246. |
I'm fine with closing this PR if we have other more general solutions that can supersede this one. |
@hez2010 Thank you for your contribution; I look forward to seeing other contributions that we can take (consider looking at the |
Substitute constant into loop testing variable where possible.
Example:
Codegen for
Test1
andTest2
:Another example involving array length, casting and nested loop:
Codegen for
Test
: