This is a bit misleading, the size is not a target pointer size, but a floating point size. I know they are the same on arm, but they are not the same on arm64 and x64 and the equivalent functions for those two use / 16. I would prefer defining FLOAT_REGISTER_SIZE in the cgencpu.h files and using it here and in the same arm64 and x64 methods.

Personally I would avoid the term FLOAT_REGISTER_SIZE on arm/arm64. I would prefer FLOAT_SIZE or SIZEOF_FLOAT.

Added FLOAT_REGISTER_SIZE,

I would prefer FLOAT_SIZE or SIZEOF_FLOAT.

would not it be confusing to have #define FLOAT_SIZE 16 on arm64?

I guess I didn't expect the value to be 16.

• For Arm32, a float register i.e. s0 would be 4 bytes, but it would be one of many in the q0/v0 register.
• For arm64 s0 is the lower 4 bytes of the v0 SIMD register which is 16 bytes.

FLOAT_SIZE is definitely, worse, but I still think FLOAT_REGISTER_SIZE might be ambiguous for ARM architectures. VECTOR_REGISTER_SIZE might be better for ARM/ARM64.

The thing is that we use m_idxFloatReg, m_cFloatReg fields in the surrounding code, thus the FLOAT_REGISTER_SIZE matches those.

This variable name is misleading, as it looks as if it was the size of argument in bytes, but is it the size aligned to stack element size. The more I look at the code below, the more I feel like we should just keep using the cSlots except for the places where we place arguments on stack (we set the m_byteStackSize). This comment is meant for all architectures.

Does cSlots meant slots count? Does slot size depend on context, like it could be a general reg size, float reg size, stack slot size?

cSlots meant slot count where slots were considered both register slots in the transition block and stack slots. Only on ARM32 the term has leaked into the floating point register count setting, which may be misleading. Now the stack slot concept is kind of broken by the Apple ARM64 calling convention, but the register slot still holds.

I have changed this code so we don't create cSlots and don't call StackElemSize unless we put byteArgSize on the stack.

I am not sure if this check is needed here. The ArgIteratorTemplate<ARGITERATOR_BASE>::GetNextOffset() is responsible for getting the right offset depending on the available registers and argument type and size.

There are cases where we get into this check, for example, JIT\Stress\ABI\pinvokes_d:

CoreCLR!ArgIteratorTemplate<ArgIteratorBase>::GetArgLoc+0x10c
CoreCLR!GenerateShuffleArrayPortable+0x27c
CoreCLR!GenerateShuffleArray+0x14

comes here with byteArgSize = 17 and we change its size to 8 here, if we don't do this we will get a wrong result.

I don't think we should make this change and the one in the caller when we can only handle the case when the offsets are multiples of TARGET_POINTER_SIZE anyways. I believe it would require quite complex change to the shuffle thunk generation to make it work for non-aligned stuff, so it seems that doing it half way doesn't bring any benefit.

It is now exactly half-way and it is required to pass existing tests.
We have cases on arm64 apple when we return byteIndex that is not byteIndex % 8 == 0 but it is the same for source and destination, so we don't need to generate a move and don't hit the NYI assert.

Ah, ok, makes sense then.

A nit - can you please use the floatRegOfsInBytes constant here too?

My bad, thanks for catching