Old code was broken, it was setting Current to gtCallArgs instead of gtCallObjp which made no sense. I don't think this code is ever hit, it's only used when calls are cloned and that's not exactly common. The only case I know about is loop hoisting but only a few helper calls can be considered invariant and hoisted. And those helper calls do not have this...

Speaking of this, I find it extremely odd that it's stored separately from rest of the args, it gtCallObjp. It complicates things and as seen here this can lead to potential bugs.

I was a bit confused by:
Use* gtCallArgs; // The list of arguments
and
UseList Args()
they are both lists but they are used differently, there are few places where we access gtCallArgs and it takes me a few secs to understand why.
We could think of how to separate them better.

Well, I would very much prefer to make gtCallArgs private but there are a lot more places that need to be changed to achieve that. I also don't like the "manual" linked list manipulation that's all over the place but that again requires a lot more changes.

I hope I'll be able to make other improvements around GenTreeCall data structures (get rid of GTF_LATE_ARG, ReplaceCallOperand, GT_ARGPLACE and gtCallLateArgs) and then perhaps do some more polishing as well because some things would just be easier.

Here it's probably also worth pointing out that keeping the args in an array rather than a linked list would make parts of the code cleaner. Unfortunately we have a few places were we need to insert "hidden" args so keeping the existing linked list approach was simpler and safer.

Nice catch, looks like the code here is old and the comment is not valid anymore, because we are not counting anything. The same for lsraxarch.

// Return Value:
//    The number of sources consumed by this node.
//
int LinearScan::BuildCall(GenTreeCall* call)

Is it the number of registers consumed by this node?
@CarolEidt ?

Right, I forgot to mention this part and the similar one in lsraxarch.cpp. It does look like at least the comment is out of date and maybe the code is no longer necessary as well. But it doesn't seem like a good idea to remove asserts while doing a large refactoring so for now I made only minimal changes.

}
else 
{

?

Hmm, don't know. I usually don't like to add else when the if branch returns. Besides, the subsequent scoping is only required because we're in a switch. Perhaps I should just pulled out all the comapre code in a GenTreeCall::Equals function like I did for PHIs.

I agree that this needn't be an else, and it might be nice to pull it out, but I don't feel that's necessary.

GitHub doesn't really support reviewing changes in a file with more than 12000 lines, especially at the end of such files. It shows me that args is unused here and you have to check the raw file to see the use.

Ha ha, didn't noticed that an entire section of the file is actually missing from the diff. GitHub is great in many ways but there's still room for improvement :)

I should have reverted this variable declaration change. Initially I did more cleanup but then I saw the size of the PR I reverted a bunch of stuff but missed a few cases. Oh well, I absolutely hate variables declared on the other side of the planet. This is probably one the worst "features" of the old C versions that required variables to be declared upfront.

GitHub doesn't really support reviewing changes in a file with more than 12000 lines

Maybe we need to figure out how to break this file up a bit!

While this method didn't have a header to begin with, it would be good to add one, especially since you've added an arg here.

I added comment headers to all affected assertionprop functions. However, I also cleaned up things a bit because I don't want to waste my time adding comments about useless overloads and parameters - assertion parameter isn't needed because everyone just passes the address of a local variable created solely for this purpose.

Of course, it would be even better to split optCreateAssertion into smaller functions such that helperCallArgs parameter isn't needed but that doesn't belong in this PR.

I agree that this needn't be an else, and it might be nice to pull it out, but I don't feel that's necessary.

GitHub doesn't really support reviewing changes in a file with more than 12000 lines

Maybe we need to figure out how to break this file up a bit!

It might be worth updating the function header of this method to clarify that both additionalTreeToBeEvaluatedBefore as well as the argList will be examined for side-effects. (Also the comment needs to be changed to use additionalTreeToBeEvaluatedBefore (singular 'Tree').

Added comment header and renamed some parameters in the process.

• additionalTreeToBeEvaluatedBefore is unnecessarily long as "before" isn't really relevant - the order in which various trees are searched for side effects doesn't matter.
• variableBeingDereferenced was misleading, there was no guarantee that it is actually a variable. That's checked in impInlineIsThis.

Pre-existing, but it would be worthwhile to mention in the function header that the hidden argument is always add just after the return buffer, if there is one, and otherwise prepended to the list.

It can also be appended to the list depending on USER_ARGS_COME_LAST. I believe that the function would be clear enough and did not require a comment, if it weren't for the "manual" linked list manipulation that happens in the ret buffer arg case. So I cleaned up the "manual" linked list manipulation instead of adding a comment. Anyway it was messed up - instead of simply inserting a new use for the hidden argument it was also recreating the use of the ret buffer arg.

It took a second look to determine that, by construction, insertionPoint cannot be null. It would definitely be worth a comment to that effect (at first I thought it merited a noway_assert until I realized that couldn't happen.

I agree that this kind of code isn't easy to read but it's only 3 lines and this is not the only place where it occurs so I don't think adding a comment is a solution. The solution would be to add a function that does this operation. I considered adding something like gtAppendNewCallArg but I don't think it's a good idea due to its potential for misuse - it has to search for the end of the list every time it is called so if it ends up being used to append multiple arguments it's not going to be good for perf.

I hope to make something better once I also change GT_FIELD_LIST to not be a list. It's a bit of a problem to abstract linked list operations in this case (single linked list with only the head being stored) but there are some ways to do this that are better than having this kind of low level list manipulation code all over the place (something similar to a back_inserter STL iterator could help for example).