RyuJIT: Don't insert GT_COMMA for x*2->x+x opt before optHoistLoopCode phase#38083
RyuJIT: Don't insert GT_COMMA for x*2->x+x opt before optHoistLoopCode phase#38083AndyAyersMS merged 5 commits intodotnet:masterfrom
Conversation
|
"Cast" seems also is not hoisted: public class Program
{
static void Test(object o)
{
for (int i = 0; i < 1000; i++)
Consume((Program) o); // cast o to Program is not hoisted from the loop
}
[MethodImpl(MethodImplOptions.NoInlining)]
static void Consume(Program p) {}
} |
|
GT_MOD by constant also inserts a GT_COMMA in the global morph phase and prevents the following code to be optimized; static void Test(int x)
{
for (int i = 0; i < 1000; i++)
Consume(-x % 10); // -x % 10 is not hoisted out of the loop
}
[MethodImpl(MethodImplOptions.NoInlining)]
static void Consume(int x) {} |
AndyAyersMS
left a comment
There was a problem hiding this comment.
Cast can cause exceptions, so hoisting is problematic.
How hard is it to fix the GT_MOD case?
src/coreclr/src/jit/morph.cpp
Outdated
| // if op1 is not a leaf/local we have to introduce a temp via GT_COMMA. | ||
| // Unfortunately, it's not optHoistLoopCode-friendly yet so let's do it later | ||
| // in the Rationalization phase. | ||
| if (!needsComma || (mostRecentlyActivePhase > PHASE_HOIST_LOOP_CODE)) |
There was a problem hiding this comment.
Does this actually kick in later? If so you might instead check for something like
comp->fgOrder == Compiler::FGOrderLinear
as mostRecentlyActivePhase is something we use for diagnostics but not to influence jit behavior.
There was a problem hiding this comment.
@AndyAyersMS it does for the snippet in the description. that MUL is visited from RewriteIntrinsicAsUserCall during the rationalization phase (Exp is not intrinsic so we convert it back to a normal call and call fgMorphArgs for the arg). However, it won't be visited for:
static void Test(double x)
{
for (int i = 0; i < 1000; i++)
{
Consume(-x * 2); // Math.Exp(-x * 2) should be hoisted from the loop
}
}
[MethodImpl(MethodImplOptions.NoInlining)]
static void Consume(double x) { }the expression will be hoisted but jit won't re-visit that node during rationalization.
AndyAyersMS
left a comment
There was a problem hiding this comment.
LGTM. Thanks for fixing this!
Fixes #37802
Try to avoid GT_COMMA insertion before
optHoistLoopCodephase to let it hoist such expressions first. A minimal repro from #37802:Current codegen:
New codegen:
Diff: https://www.diffchecker.com/y6dL37Qi
/cc @AndyAyersMS