Keep precise argument sizes between import and morph.

[sandreenko]

On arm64 OSX we need to know precise argument sizes. I have added support for that between morph and lowering in #42503, this PR supports in between importation and morph.

When do we need it:

IL: load byte; call1(int) -> IR: CALL1(IND byte); IL load int; call2(byte) -> IR: CALL2(IND int); 

without the change, morph was seeing the incorrect type of the nodes but for other platforms it was not important (because all arguments were passed in TARGET_POINTER_SIZE slots).

I have stopped on the solution that adds a special GT_OPER to keep this information between these phases and a debug code in morph to check that it has survived phases between them (mostly inlining). Thanks @CarolEidt for the idea.
I want to enable this code for all platforms because:

1. do not want to introduce platform dependency in HIR;
2. it will make arm64 OSX work cheaper because we will catch bugs in this component on other platforms until we have arm64 OSX in ci;
3. the overhead is small, it should not affect TP;

I was also considering other solutions, the main 2:

1. add a new artificial structure to keep this information, map/dictionary, but

• we can't increase call node size;
• it will be an error-prone component from our previous experience with fgArgInfo;
• it will have non-const access time or memory overhead, also non-trivial copying;

2. change impImplicitIorI4Cast to insert cast not only between i4 and i8 but between all sizes, but

• we don't need it if it is not under a call;
• it will require changes during inlining to ignore such nodes;
• it will be easy for such nodes to slip through morph phase;

No asm diffs (SPMI x64/x86). Update: ci shows related failures during the check phase, I will fix them soon, but I think it should not block review.

[sandreenko]

I wonder why we did not see a warning about an assignment inside if.

[sandreenko]

callSig == nullptr means mostly a helper call, all helper calls that I have checked are working with i4 and i8 types but I expect I will have to add more PUTARG_TYPE there once I run more tests on arm64 OSX and find bugs with missed type information.

[sandreenko]

PTAL @CarolEidt @AndyAyersMS @dotnet/jit-contrib

[AndyAyersMS]

I'm puzzled why we need to do things this way -- doesn't the callee sig tell us what size the callee expects?

[sandreenko]

I'm puzzled why we need to do things this way -- doesn't the callee sig tell us what size the callee expects?

We get this information from callee sig, but it is available only during importation and then we lose it. In debug we save it in

#ifdef DEBUG // Used to register callsites with the EE CORINFO_SIG_INFO* callSig; #endif 

if we decide to keep it in Release it will increase the size of all large nodes.

[AndyAyersMS]

Seems like we could deal with that easily enough, we have various extensions already hanging off of calls. For instance we could refactor TailCallSiteInfo which already holds onto a CORINFO_SIG_INFO and use it more broadly for cases where we need the sig later in compilation.

Is there other benefit to having these extra IR nodes beyond being able to handle this ABI quirk?

[sandreenko]

Is there other benefit to having these extra IR nodes beyond being able to handle this ABI quirk?

These extra nodes are only for the ABI quirk, I do not see benefits to other places from it. I was thinking to postpone casts creations but it was a long shot (if we pass 1 byte as 1 byte we don't need CAST int<-byte<-int).
I was using the signature in my prototype (from the debug field) to get the correct handle and there were some issues:

1. no way to handle calls without a signature if we get such;
2. not easy to match the signature to the actual argument nodes in morph. I am trying to do it for checking now, but with additional arguments from both sides of the list it is not trivial;
3. need to do extra calls to strip(info.compCompHnd->getArgType(sig, sigArgs, &classHnd)) (minor);

[CarolEidt]

Some initial comments and suggestions

[CarolEidt]

I'm curious why you added this here - that is, I can see why we wouldn't expect to encounter these under a COMMA, but I'm not sure why this special assert is needed.

[sandreenko]

We have several similar functions like gtEffectiveVal, gtRetExprVal and gtSkipPutArgType. When we call each of them we often don't expect nodes that can be skipped by two others to be on top.
For example, when we call gtEffectiveVal we don't expect something like GT_RET_EXPR(GT_COMMA) or GT_PUTARG_TYPE(GT_COMMA), similar for gtRetExprVal it should not see GT_PUTARG_TYPE(GT_PUTARG_TYPE).
If we do it means the caller did not check for these nodes and we don't skip what we hoped to skip, it doesn't cause correctness issues in my understanding but introduces asm regressions when we can't parse something like we did before the change.

[CarolEidt]

Should this be !=? Otherwise, the loop below will never execute, right?

[sandreenko]

It is a temporary condition, the idea here is to match the signature with the call arguments as we have them in morph and check that we still have the correct types. However, it is tricky to do because we could have some nodes added to gtCallArgs between importation and call to this function (also this function can be called several times).
So we need to skip all additional nodes that were added and I have problems with identifying them. I am planning to fix it once I pass all the other tests.

[sandreenko]

/azp run runtime-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[sandreenko]

/azp run runtime-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[sandreenko]

The PR was updated and is passing the tests now.
Thanks to @AndyAyersMS for helping me offline with the previous test failures.

The changes include #44790 that I hope to merge first.

This is the last big JIT change for arm64 apple, with it we will have precise arguments sizes from importer to codegen. The next changes will be on VM side for interop stubs.

[sandreenko]

The main change is that we save PUTARG_TYPE and RET_EXPR in inlCurArgInfo->argNode.

[sandreenko]

int32 is passed as int16 to Inline1 where it is passed to CallAMethod1, before the last changes the saved node was LCL_VAR int and we added another PUTARG_TYPE on top of an existing one, now it is fixed.

[sandreenko]

This new condition allows us to always the argument as a temp with the correct type, we need it because for a correct direct substitution we need to know if the parent is a call or not but we don't have this information.
We create the node when we see an IL opcode like ldlocal and don't have parent information, later, when the node is used, we can't distinguish it from a regular LCL_VAR.
The change causes a small regression on all platforms:

windows x64 crossgen: 5134 (0.02% of base) windows x64 pmi: 14911 (0.03% of base) windows arm64 crossgen: 5688 (0.01% of base) linux x64 crossgen: 9036 (0.02% of base) 

, etc. all lower than 0.04%.

What happens there is:

1. during importation we create a LCL_VAR tempForOurArg, save a pointer to it;
2. in fgInsertInlineeBlocks we iterate over all such pointers and do in-place replacement if there was only one use of the lclVar, but we don't know the user, so we can't say if it is a call that need a PUTARG or not.

I have not found a simple way to avoid it or compensate for it.

[AndyAyersMS]

It would be good to have a comment in the code here explaining why argHasPutArg disqualifies this arg from direct substitution.

[AndyAyersMS]

Left you some questions about this change.

[AndyAyersMS]

Do you need this change for correctness? If not perhaps you should PR it separately.

[AndyAyersMS]

Ditto for the other two below.

[sandreenko]

I do not need this for correctness. I am planning to merge them in #44790 (comment), if we decide not to merge it I will need to change this code to use argNode->gtSkipPutArgType()->gtRetExprVal().

[AndyAyersMS]

I just approved #44790.

[AndyAyersMS]

It would be good to have a comment in the code here explaining why argHasPutArg disqualifies this arg from direct substitution.

[AndyAyersMS]

See note above; I don't believe you can set these to zero like this and pass jit stress.

[sandreenko]

/azp run runtime-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[AndyAyersMS]

I think this is good to merge once you've updated for #44790 and perhaps added a comment indicating why PUTARG_TYPE can't be directly substituted.

[sandreenko]

I have checked that the diffs are still as expected before the merge, PR and jitstress are clear,
the only ci failure is due to #45168.

[sandreenko]

Thanks @AndyAyersMS and @CarolEidt for your help with this change.