Tagging subscribers to this area:@JulieLeeMSFT
See info inarea-owners.md if you want to be subscribed.

Shouldn't we just remove early expansion of UMOD/MOD for arm64 from morph and use the shared (with x86) impl in lower instead?

@EgorBo are you referring toLowering::LowerConstIntDivOrMod ?

LowerConstIntDivOrMod

yeah, and just movemod toa % b = a - (a / b) * b; there - thus, we won't have to re-implement the already existing optimization.

One potential problem with this approach that it might produce regressions wherea - (a / b) * b previously led to more CSE opportunities, e.g. witha / b next toa % b

It makes sense that we should do it there so the earlier phases don't screw it up.

I did some work to see if I could move the existing mod optimizations to lowering, but it might be a bit much for what the PR is trying to accomplish.

Shouldn't we just remove early expansion of UMOD/MOD for arm64 from morph and use the shared (with x86) impl in lower instead

@EgorBo, I would've thought it was better to do it early in morph so other things can more easily take advantage of the optimization (we don't optimize arounddiv/rem in many cases)?

@EgorBo, I would've thought it was better to do it early in morph so other things can more easily take advantage of the optimization (we don't optimize around div/rem in many cases)?

I agree.

Shouldn't we just remove early expansion of UMOD/MOD for arm64 from morph and use the shared (with x86) impl in lower instead

@EgorBo, I would've thought it was better to do it early in morph so other things can more easily take advantage of the optimization (we don't optimize arounddiv/rem in many cases)?

cc@kunalspathak@tannergooding

I personally think it's not, for any non-leaf X inX % Y we have to introduce a new local (ASG node) instead of just keeping a simplex mod y expression, e.g(x + 1) % y early in morph is converted into:

[000015] -A-X-+--R---              \--*  SUB       int   [000012] -----+------                 +--*  LCL_VAR   int    V03 tmp1         [000014] -A-X-+------                 \--*  MUL       int   [000004] -A-X-+------                    +--*  DIV       int   [000011] -A---+------                    |  +--*  COMMA     int   [000009] -A---+------                    |  |  +--*  ASG       int   [000008] D----+-N----                    |  |  |  +--*  LCL_VAR   int    V03 tmp1         [000002] -----+------                    |  |  |  \--*  ADD       int   [000000] -----+------                    |  |  |     +--*  LCL_VAR   int    V00 arg0         [000001] -----+------                    |  |  |     \--*  CNS_INT   int    1[000010] -----+------                    |  |  \--*  LCL_VAR   int    V03 tmp1         [000003] -----+------                    |  \--*  LCL_VAR   int    V01 arg1         [000013] -----+------                    \--*  LCL_VAR   int    V01 arg1

instead of just:

[000004] ---X--------              \--*  MOD       int   [000002] ------------                 +--*  ADD       int   [000000] ------------                 |  +--*  LCL_VAR   int    V00 arg0         [000001] ------------                 |  \--*  CNS_INT   int    1[000003] ------------                 \--*  LCL_VAR   int    V01 arg1

E.g. it makes it non-hoistable for ARM64, e.g. see this:

loops are highlighted for both x64 and arm64

@EgorBo, I was referring specifically to thex % SomePow2 optimization being introduced here.

It should be a clear improvement to recognize and replacex % SomePow2 withx & (SomePow2 - 1) since that's the same number of nodes, still a constant, but alsoAND is better understood and optimized thanDIV orMOD

@EgorBo, I was referring specifically to thex % SomePow2 optimization being introduced here.

It should be a clear improvement to recognize and replacex % SomePow2 withx & (SomePow2 - 1) since that's the same number of nodes, still a constant, but alsoAND is better understood and optimized thanDIV orMOD

I'm fine with doingx umod POT early in morph - it makes sense and doesn't produce additional local, I was referring to my suggestion to remove the early expansion of generalX [u]mod Y

@kunalspathak @echesakovMSFT This is ready.

Will try to restart CI.