xplat: use xmm for float args

[obastemur]

Previously xplat LowerCall args were limited to REG_INT_ARG... Adding XMM* support and a test case to make sure we don't break.

This PR also improves performance on xplat jetstream

Disabling couple of additional wasm tests on xplat with a note.

edit: added another test case

[obastemur]

@dotnet-bot test Windows ci_slow_x64_debug please

[jianchun]

This is a pretty neat idea 👍 I'm worried about the locations of Start/Stop recording though. It seems to me maybe every GetArgSlotOpnd call needs to be recorded. We use a mixture of LowerCallArgs and several other random code paths to load different args, which may all need to be homed. E.g., even the helperCallArgs loaded in LowerCall may need to be homed. Stopping here may miss some args. Please survey all calls to GetArgSlotOpnd to see if this is a potential issue.

[obastemur]

if we count them all, we would also count the ones we endup pushing to stack. We are only interested with the ones explicitly located on registers (float and non-float) . The rest, we consider them as non-float.

[obastemur]

even the helperCallArgs loaded in LowerCall may need to be homed.

Indeed they are but we don't need to check/store their type.

[jianchun]

No, we only record register args, which is what you currently doing inside GetArgSlotOpnd. You are not recording stack args.

Recording all might be more clear, otherwise you have to assume this is the only place xmm args could be loaded, all others in int registers, other registers may have no recordings, making the homing code complicated... (If recording all is hard to do right, this is fine with me)

[obastemur]

If you follow the usages of GetArgSlotOpnd you will see that there are 2 places setting argSym so the register can be anything but TyMachReg. One of them we record. Other, we don't have to since shouldHomeParams = argCount > 0 and !shouldHomeParams... So we record them all right.

[jianchun]

I'm confused by the logic here -- could it be due to the possibility that you were not recording all args loading (my other comment)? If you recorded every arg loading, the logic here could be very simple. Every arg from position 0 to xplatCallArgs.topPosition must be in register, type recorded, and must be homed. You can then figure out the register from position and your recorded type and home it. Seems no need to count int/float registers.

[obastemur]

I wanted to limit recording to actual registers. This way we know / store only the ones make sense for our homing.

[jianchun]

Assuming you can guarantee that the partial recording successfully records every float register arg, the logic here seems quite complex and could contain bugs. E.g., given 8 args: int0, int1, int2, int3, int4, int5, int_stack, xmm7.

callArgCount == 8
xpTopPos == 8
floatCount == 1
argRes == 8
intCount == 6
argRegs == 7

Then your following for loop will start from i = 8 and end at i == 2 (loop 7 iterations because of argRegs). You'll miss homing the 1st register int0.

Seems it could be simpler:

for (int i = min(callArgCount, XmmArgRegsCount); i > 0; i--) {
  if (had recording for i and it is float) {
    prep homing float reg
  } else if (i <= IntArgRegsCount) {
    prep homing int reg
  }
  ...
}

[obastemur]

1 ) callArgCount will be at least 9 -> you are missing the +call_info.
2 ) in your case i won't represent the the arg slot index..
3 ) good test case idea, will update the one on this PR.

[jianchun]

callArgCount will be at least 9 -> you are missing the +call_info.

callArgCount is total native arg count, not original runtime arg count -- in my example "int1" is call_info.

in your case i won't represent the the arg slot index..

Why? i does represent the arg slot index.

[obastemur]

if (had recording for i and it is float)
Why? i does represent the arg slot index.

As long as we loop reset the whole array each time and empty loop till first int register...

Anyways, updated the PR.

[jianchun]

Seems all calls to GetArgSlotOpnd is to load an argument, and if we are to record every one, IsRecording() should be true.

[jianchun]

style nit: inline is assumed when implementation is in class declaration. We usually don't write inline here explicitly.

[obastemur]

inline is assumed when implementation is in class declaration.

Sorry, how this implementation is different ?

[jianchun]

I was just proposing removing inline keyword here.

[obastemur]

Got that, could you please clarify why remove inline keyword here?

[jianchun]

Personal preference, as I think it is redundant and gets in the way of code reading.

[jianchun]

nit: Here it assumes XmmArgRegsCount >= IntArgRegsCount. Could use some code to guard that.

[jianchun]

The new tests are fine, I'm just thinking maybe existing tests already covering this? The reason we didn't hit this was just because we were not running all existing tests (-simdjs issue). If you change runtests.py, remove that flag, maybe you can see the difference?

[obastemur]

maybe but we don't have simd is enabled. These tests are now useful for xplat and will be useful for other archs.

[jianchun]

There are some assumptions hidden in this code. Looks this code assumes the recording pattern must be we always record every register in the back range but none in the front range if any is missing recording. That's why you clear range [0, start). Is it right? Is there a way to ensure this assumption holds? At least can you document it?

[jianchun]

nit: floatCount tracked but not being used.

[obastemur]

@jianchun Thanks for the review. Made some additional improvements.