I understand that you have modelled your changes as done for unix_amd64. I don't really like the way this has been implemented for amd64_unix. I dont think above needs to be done here in GetNextOffset(). GetNextOffset() only returns the offset in TransitionBlock. In order to get ArgLocDesc we have another function GetArgLoc(). So first you get the offset and then use that to get ArgLocDesc. This would also eliminate the need for fields m_hasArgLocDescForStructInRegs & m_argLocDescForStructInRegs in ArgIterator which I think is not required.

@rahku the reason why I have implemented it this way for unix amd64 is that there is no single offset in the TransitionBlock for structs passed in registers on Unix amd64. A struct can be split between upto two floating point and two general purpose registers. In this case, we return a special offset value TransitionBlock::StructInRegsOffset. I have originally refactored the code to let the GetNextOffset return a bitfield that would contain both the floating point registers and general purpose registers offsets, but it turned out to complicate code in many callers of the function and so it was decided during the code review to revert that change and rather use this side storage for these details.

But unless ARM64 can also split a single argument between multiple general purpose and floating point registers, I agree with you that we should use a single offset like we do e.g. for AMD64 on Windows.

changing above to
#if (defined(UNIX_AMD64_ABI) && defined(FEATURE_UNIX_AMD64_STRUCT_PASSING)) || defined(TARGET_ARM64)
will make it clear what the real conditions are

I don't think this is a good design. I would not do it for arm64. I have explained details below.

shouldn't function "void ZeroStructInRegisters(int fieldBytes)" also be moved out of ifdef arm64

Addressed

Why is this different for ARM64? It seems you can make it the same and then you would not need the change in the argdestination.h in IsStructPassedInRegs.

Are you suggesting to add fields m_hasArgLocDescForStructInRegs & m_argLocDescForStructInRegs in ArgIterator for arm64 as well? These are not required for any other architecture except unix amd64. It is possible on arm64 to get ArgLocDesc with just the offset address. This is how it is for other architectures as well you first get the offset and then get ArgLocDesc. As you said that on amd64 unix that is not possible so I would let the special case be just for amd64 unix and let the logic for arm64 be same as other arch.

No, I was suggesting that to keep the code uniform for all platforms. All platforms now have ArgIterator::GetArgLocDescForStructInRegs and it just always returns NULL on non-amd64. So it seems strange to deviate from that for ARM64.

I don't understand why we need to handle this differently on ARM64 and why NULL m_argLocDescForStructInRegs cannot indicate that there is no struct passed in regs.

for amd64 we seem to always be setting m_argLocDescForStructInRegs for all arguments irrespective of whether the offset corresponds to struct_in_regs....which i don't think is right. You could refactor amd64 unix code to not do that and then this can be made same for both arch.

That's not the case. It is set only for structs in registers, for others, it is NULL. The ArgDestination::m_argLocDescForStructInRegs is set in the constructor and we pass ArgIterator::GetArgLocDescForStructInRegs result to the constructor. And the ArgIterator::GetArgLocDescForStructInRegs looks like this:

    ArgLocDesc* GetArgLocDescForStructInRegs()
    {
#if defined(UNIX_AMD64_ABI) && defined(FEATURE_UNIX_AMD64_STRUCT_PASSING)
        return m_hasArgLocDescForStructInRegs ? &m_argLocDescForStructInRegs : NULL;
#else
        return NULL;
#endif
    }

So it only returns non-NULL if the argument is a struct passed in registers.

by "you could" i meant "one would need to "

I'm not sure I understand your comment. I've just shown that we are not setting m_argLocDescForStructInRegs for all arguments.

never mind regarding the above comment. Yes I now see that you re-initialize m_hasArgLocDescForStructInRegs to be false at the start of every call to ArgIterator::NextOffset(). So yes this function can be made to
return m_argLocDescForStructInRegs != NULL;
for all applicable arch (amd64 & arm64)

Why do we zero the memory when we use 64 bit stores in the loop below that overwrite all the zeroed bytes anyways?

This was mostly used when debugging, thank you for catching it. Will remove.

I still wonder why we have to differ for ARM64 here. Based on the IsHFA function above, the ArgDestination should have the m_argLocDescForStructInRegs set to NULL for non-HFA args. So if you used the original code at this place and just made sure that m_hasArgLocDescForStructInRegs in the ArgIterator is set only for HFA args, it seems it should work.

Yes and that seems like unification with unix amd64 which we decided to do later and not as part of this work.

@jashook I am sorry for the delay, I was OOF for the last four days. Looking at the change again, I am not sure I understand one thing related to my last comment here. It seems that IsHFA now always returns true, since you always pass the ArgDestination constructor a non-null argLocDescForStructInRegs argument. Is that correct?

It seems that you can do even this the way it is done for amd64. Instead of having this lambda and calling it from three code paths below, it seems you can just add code to initialize the m_argLocDescForStructInRegs inside the if (thValueType.IsHFA()) under the case ELEMENT_TYPE_VALUETYPE above.
At that place, instead of translating the just generated offset back to the specific m_idxXXXReg, you would just call m_argLocDescForStructInRegs.Init() and set the m_argLocDescForStructInRegs.m_idxFloatReg to m_idxFPReg and m_argLocDescForStructInRegs.m_cFloatReg to cFPRegs. Since the lambda below sets the m_argLocDescForStructInRegs for HFA only, it should get the equivalent result.
It looks the change would also be simpler. I hope I have not missed something.

The type information was missing.