Introduce insGroup/instrDesc backwards navigation

[BruceForstall]

Currently, it is only possible to walk forwards in insGroup/instrDesc data: insGroup are in a single-linked list, and variable-sized instrDesc are packed into a memory array where function emitSizeOfInsDsc() can tell you the size of an instrDesc, and emitAdvanceInstrDesc() can advance an instrDesc* forward, but there is no facility for walking to the previous instrDesc.

This change introduces a igPrev pointer so the insGroup list is doubly-linked. Also, each instrDesc is annotated with 4 or 5 bits of data indicating how large the previous instruction in the buffer is, so it is possible to find it by subtracting that size from a instrDesc*. Also, each insGroup maintains an igLastIns pointer to the last instruction in the group, to allow seeding the backwards walk.

The instrDesc backwards walk data is taken from the "small constant" area, so some small constants get pushed to using larger instrDesc sizes.

Walking backwards is done using emitGetLastIns() to find the last instruction in the function, and emitPrevID() to walk to the previous one.

This information is currently only used in the JitDump.

Basic measurement of the benchmarks SPMI collection showed about a 2.5% increase in emitter memory usage.

[ghost]

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

Issue Details

---

Currently, it is only possible to walk forwards in insGroup/instrDesc data: insGroup are in a single-linked list, and variable-sized instrDesc are packed into a memory array where function emitSizeOfInsDsc() can tell you the size of an instrDesc, and emitAdvanceInstrDesc() can advance an instrDesc* forward, but there is no facility for walking to the previous instrDesc.

This change introduces a igPrev pointer so the insGroup list is doubly-linked. Also, each instrDesc is annotated with 4 or 5 bits of data indicating how large the previous instruction in the buffer is, so it is possible to find it by subtracting that size from a instrDesc*. Also, each insGroup maintains an igLastIns pointer to the last instruction in the group, to allow seeding the backwards walk.

The instrDesc backwards walk data is taken from the "small constant" area, so some small constants get pushed to using larger instrDesc sizes.

Walking backwards is done using emitGetLastIns() to find the last instruction in the function, and emitPrevID() to walk to the previous one.

This information is currently only used in the JitDump.

Basic measurement of the benchmarks SPMI collection showed about a 2.5% increase in emitter memory usage.

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

[BruceForstall]

Related: #80589

[BruceForstall]

There's an assertion building SPC.dll on ARM:

Assertion failed 'idPrevSize() == prevInstrDescSizeInBytes' in 'Interop+Sys:.cctor()' during 'Generate code' (IL size 27; hash 0xb6a186f0; FullOpts)

[BruceForstall]

Diffs

TP results:
x64/x86: +0.04% full-opts, +0.11% MinOpts
arm64: a wash, actually a -0.03% win on MinOpts, +0.01% on full-opts

[kunalspathak]

Diffs

TP results: x64/x86: +0.04% full-opts, +0.11% MinOpts arm64: a wash, actually a -0.03% win on MinOpts, +0.01% on full-opts

That's not bad. I am just pasting the screenshot in case the build info gets deleted in future.

2.5% increase in emitter memory usage.

So, something to think about.

[BruceForstall]

Memory usage change from the "bytes allocated in the emitter" output of EMITTER_STATS:

MCH	baseline	diff	diff%
aspnet	334,750,312	354,970,200	+6.0%
benchmarks	76,073,420	80,509,356	+5.8%
coreclr_tests	2,204,391,472	2,315,013,312	+5.0%
libraries.crossgen2	439,754,744	462,642,232	+5.2%
libraries.pmi	611,205,992	646,594,872	+5.8%
libraries_tests	1,010,558,124	1,062,405,996	+5.1%

[BruceForstall]

@dotnet/jit-contrib Here's an experiment I wrote to implement full backwards navigation support in MIR (emitter insGroup/instrDesc lists). The throughput impact is minimal and the memory increase might be small enough to not matter.

[BruceForstall]

/azp run runtime-coreclr superpmi-diffs, runtime-coreclr superpmi-replay

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[BruceForstall]

@dotnet/jit-contrib PTAL. I propose to check this in under #ifdef, disabled, under the assumption we might need this or something like it in the near future. (If it ends up we don't need it, or end up with something else, it's easy enough to remove.)

[kunalspathak]

Overall looks good approach. I am wondering if we should enable a small optimization using this functionality to verify if it is working as expected?

[BruceForstall]

I am wondering if we should enable a small optimization using this functionality to verify if it is working as expected?

There are the asserts and verifications I added. Also, I think @TIHan has some ideas for something that could use this.

[BruceForstall]

@TIHan @dotnet/jit-contrib I would still like to merge this change (with the new feature disabled). Code review?

[TIHan]

Hey @BruceForstall , I'll look at thoroughly in the morning - quick glance as it looks fine to me assuming we are ok with the TP and memory costs.

[BruceForstall]

assuming we are ok with the TP and memory costs.

Note that all this code is disabled for now, so the decision about memory/TP cost will have to come along with an optimization client that wants to turn it on.

[TIHan]

Will need to use this asap for two peephole opts that I want to do.