Make intrinsic nodes multi op (aka delete GT_LIST)

[SingleAccretion]

This is the final result of the work detailed here, with one very significant addition: GTF_REVERSE_OPS is supported for the multi-op nodes (as much as I would have liked to drop that handling, 3 vector methods showed up in benchmarks with regressions since the time I originally wrote the code).

This is a zero-diff change across all configurations according to SPMI.

The history of this branch has been heavily rewritten to assist in review: there is exactly one commit for each changed function, with the exception of the first commit and the two last commits.

A few TODO-List-Cleanup comments have been added highlighting further work/simplifications enabled by this change (which will be addressed if it is accepted).

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This is the final result of the work detailed here, with one very significant detail: GTF_REVERSE_OPS is supported for the multi-op nodes (as much as I would have liked to drop that handling, 3 vector methods showed up in benchmarks with regressions since the time I originally wrote the code).

This is a zero-diff change across all configurations according to SPMI.

The history of this branch has been heavily rewritten to assist in review: there is exactly one commit for each changed function, with the exception of the first and two last commits.

Author:	SingleAccretion
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[SingleAccretion]

@dotnet/jit-contrib, @tannergooding

[AndyAyersMS]

@SingleAccretion thanks again for what looks like a very nice contribution.

@dotnet/jit-contrib who would like to be involved in reviewing this?

[echesakov]

@SingleAccretion Thank you for the contribution.

@dotnet/jit-contrib who would like to be involved in reviewing this?

@AndyAyersMS I can take a look later this week.

[JulieLeeMSFT]

@echesakovMSFT PTAL.

[echesakov]

@echesakovMSFT PTAL.

Sure, I will take a look this week, didn't have time to do it last week

[echesakov]

I feel that these should be

GenTree* GetOp(size_t index) const;
void SetOp(size_t index, GenTree* value);

The assignment like

tree->Op(1) = op1;

take at least couple seconds from me to parse.

@dotnet/jit-contrib Anyone has the same opinion?

[EgorBo]

Personally, I'm fine with both - maybe other members have a different opinion? We mostly use Get/Set pattern, but it's more verbose, and we already use rhs assignments, e.g.

[SingleAccretion]

FWIW, the reason I made these short is that I find the pattern of "op maintenance" (see here for an example) displeasing, and bug-prone (I spent at least a few hours on fixing one occurrence), so I would like to encourage people to use the accessors more liberally and let the native compiler do the CSEs for us.

Because of this I wanted it to be Op, but then it could not be GenTree* because in our codebase this is the prevailing pattern:

TThing& Thing();
TThing GetThing() const;
SetThing(TThing thing);

But I have no strong attachment to this (or arguments for it), and will happily rewrite them as we decided is best.

[echesakov]

Overall, this looks great!
I like how it is simple to handle multi-op intrinsics after the change.
I left couple comments we should address before merging this.

[echesakov]

@SingleAccretion Can you please resolve the conflicts? I would want to measure the JIT throughput impact before signing off on this.

[SingleAccretion]

@echesakovMSFT out of curiosity: what setup will you be using?

[echesakov]

@echesakovMSFT out of curiosity: what setup will you be using?

@SingleAccretion Previously, we used Pin with crossgen (v1) running on SPC. Since we don't built crossgen.exe anymore, I am going to use Pin with superpmi. As you know, Pin runs only on Intel platforms, so for Arm64 assessment I will do crossjitting. Note that given the specifics of the changes (they would primarily affect code that extensively uses hardware intrinsics) I am the most interested in running superpmi on coreclr_tests collection rather than libraries collection (but I will do both anyway). As an alternative, I might use/create a superpmi collection that has JIT/HardwareIntrinsics tests only.

[SingleAccretion]

Ehh, I was hoping that recent changes to the intrinsic code would apply cleanly, seems however like that's not the case and I will need to do more conflict resolution (it's fine, just will take some time as the base of this branch is not the base of my currently built fork)...

Edit: looks done.

[echesakov]

I measured the JIT throughput impact using the following setup: running superpmi replay under Pin tool with clrjit_win_x64_x64.dll and clrjit_universal_arm64_x64.dll running on two collections - coreclr_tests.pmi (that contains all the JIT\HardwareIntrinsics tests) and libraries.pmi (that contains all vectorized code that we have in .NET). Each combination was run three times.

The results show that there is slight improvement with this change on both x64 and arm64.

collection name	base (instr. count)	diff (instr. count)	base mean	diff mean	relative difference
coreclr_tests.pmi.windows.arm64.checked.mch	473931392931,474021284550,473784971639	472193993230,472957155424,472498138042	473912549707	472549762232	-0.29%
libraries.pmi.windows.arm64.checked.mch	309239932345,308904007474,308783025160	308501962090,309184810211,308615556152	308975654993	308767442818	-0.07%
coreclr_tests.pmi.windows.x64.checked.mch	432614447265,431224537281,433067453527	431036382674,431957329675,430281590377	432302146024	431091767575	-0.28%
libraries.pmi.windows.x64.checked.mch	289881794617,290333310434,289971314603	289876347257,289774232789,289510475775	290062139885	289720351940	-0.12%

[echesakov]

Thank you for contribution, @SingleAccretion! Great work!