Make sure that GenTree pointer is stored only in its parent.

[sandreenko]

Right now we can have fgArgInfo that has fgArgTabEntry that points to a GenTree, so every time when we replace one tree with another we have to check for this case and do an expensive linear search in the arg info array. See ReplaceOperand for example.

We should get rid of any second edges to GenTree, so every time when we replace/remove a tree we need to update only one edge.

category:implementation
theme:ir
skill-level:expert
cost:large

[sandreenko]

@mikedn I think you have mentioned that you have a PR for it, have not you?

I thought it would be useful to create an issue for it, so other issues around RemoveTree/ReplaceTree/BashOperand can reference it.

[mikedn]

Well, yes, I have an experimental change on top of GT_LIST removal that does away with the GenTree* in fgArgTabEntry and GTF_LATE_ARG that needs to be maintained in ReplaceOperand and other places - dotnet/coreclr#24294.

Tests are passing so it looks encouraging but I found there's a piece of code in lowering (profiler hook insertion) that was broken by the change and presumably we don't have test coverage for. I'll have to fix that before definitely saying that yes, we can get rid of this mess.

Besides, the change is large, I'll probably need to split it up in multiple PRs and/or scale it down a bit.

But before all that we need to finish with the loop hoisting PR so I can get to GT_LIST removal, can't finish it without that loop hoisting change.

[mikedn]

I've updated dotnet/coreclr#24294 to include only the removal of ReplaceCallOperand.

IMO we should also get rid of GTF_LATE_ARG to actually fix this issue, otherwise we'd still need to be careful when replacing operands.

But luckily that can be done separately to keep the PRs manageable.

[mikedn]

I think it may be useful to briefly explain how I see the future of GT_CALL, that may clarify some things:

• GTF_LATE_ARG should go. Removal of node helps but we still need to be careful about maintaining this flag and this happens in places other than ReplaceCallOperand (e.g. in fgMorphCopyBlock).
  • To get rid of it we'll probably need to add a fgArgTabEntry to GenTreeCall::Use so we can get back and forth between use, lateUse and the arg entry. I don't like this very much but I hope it's only temporary.
  • There's one particular place where the above won't help - LIR liveness. This brings us to another call oddity - the late arg list persists in LIR even if its sole purpose is to control operand evaluation order, something that LIR already does by itself. So we need to clear gtCallLateArgs and replace gtCallArgs placeholders with the real nodes during rationalization.
• And speaking of gtCallLateArgs and ordering - this "late arg" invention is probably the worst possible way to control arg evaluation order. Not the list itself (which is kind of natural to have - you need to have a way to maintain 2 argument orderings) but the placeholders. Instead of simply keeping the same Use in 2 lists a new Use is created and the original Use's node is replaced by a NOP/ARGPLACE. These nodes are on about the same level of badness as LIST nodes - waste of memory and cycles, special cases that don't fit well in various analysis/transforms (can't CSE, require special value numbering etc.)
• And if you get rid of "late args" then there's probably little point in having fgArgTabEntry around. Today it's useful because it is shared between the "arg" and "late arg". If "late arg" goes then the information from fgArgTabEntry could probably be stored directly in Use.
• And there's the this arg which is not clear why it's not included in the normal arg list. As is now the only thing it seems to contributes is extra code for its processing.

So in summary - how would a GenTreeCall with a single list of all arguments would look like. No "late args", no placeholders, no arg tables.

We'll see. I have the GTF_LATE_ARG removal mostly done but the rest is only in my imagination 😁

[sandreenko]

@mikedn there are 2 class Use now that looks very similar (GenTreeCall, GenTreePhi), can they be merged?

[mikedn]

@sandreenko Eventually yes. And the one from field list should inherit from a common one. Need to finish the removal of GT_LIST from GT_SIMD/HWIntrinsic first to decide how exactly it's all going to look. In the SIMD case I would very much prefer to not have an array of uses rather than a linked list.

[jakobbotsch]

This was fixed as a side effect of #67238.