Test Failed: Assert failure: 'sz == _idCodeSize'

[VincentBu]

Job:
https://mc.dot.net/#/user/coreclr-outerloop-jitstress2-jitstressregs/ci~2Fdotnet~2Fcoreclr~2Frefs~2Fheads~2Fmaster/test~2Ffunctional~2Fcli~2F/20190609.1/workItem/JIT.Regression.CLR-x86-JIT.V1-M09-M11/analysis/xunit/JIT_Regression._CLR_x86_JIT_V1_M09_5_PDC_b25463_b25463_b25463_~2F_CLR_x86_JIT_V1_M09_5_PDC_b25463_b25463_b25463_cmd

Failed tests:
JIT_Regression.CLR_x86_JIT_V1_M09_5_PDC_b25463_b25463_b25463._CLR_x86_JIT_V1_M09_5_PDC_b25463_b25463_b25463_cmd

Log:

Assert failure(PID 3192 [0x00000c78], Thread: 408 [0x0198]): Assertion failed 'sz == _idCodeSize' in 'ILGEN_0x7122ba20:main():int' (IL size 171)

    File: f:\\workspace\\_work\\1\\s\\src\\jit\\emit.h Line: 888
    Image: C:\\dotnetbuild\\work\\6c91f97e-ceba-49f1-b03d-10156f43cafe\\Payload\\CoreRun.exe

Return code:      1
Raw output file:      C:\\dotnetbuild\\work\\6c91f97e-ceba-49f1-b03d-10156f43cafe\\Work\\c71234d0-eff6-4156-98e3-78836636de0c\\Exec\\JIT\\Regression\\Reports\\JIT.Regression\\CLR-x86-JIT\\V1-M09.5-PDC\\b25463\\b25463\\b25463.output.txt
Raw output:
BEGIN EXECUTION
 \"C:\\dotnetbuild\\work\\6c91f97e-ceba-49f1-b03d-10156f43cafe\\Payload\\corerun.exe\" b25463.exe 
Expected: 100
Actual: -1073740286
END EXECUTION - FAILED
FAILED
Test Harness Exitcode is : 1
To run the test:
> set CORE_ROOT=C:\\dotnetbuild\\work\\6c91f97e-ceba-49f1-b03d-10156f43cafe\\Payload
> C:\\dotnetbuild\\work\\6c91f97e-ceba-49f1-b03d-10156f43cafe\\Work\\c71234d0-eff6-4156-98e3-78836636de0c\\Exec\\JIT\\Regression\\CLR-x86-JIT\\V1-M09.5-PDC\\b25463\\b25463\\b25463.cmd
Expected: True
Actual:   False

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

[BruceForstall]

Looks like Windows/x64, JitStress=2, JitStressRegs=1/4/8

[BruceForstall]

@dotnet/jit-contrib

[BruceForstall]

Also seen in corefx tests, System.Data.Common.Tests, Windows/x64, JitStressRegs=4.

https://mc.dot.net/#/user/coreclr-corefx-jitstressregs/pr~2Fdotnet~2Fcoreclr~2Frefs~2Fpull~2F24962~2Fmerge/test~2Ffunctional~2Fcorefx~2F/20190610.2/workItem/System.Data.Common.Tests/wilogs

[sandreenko]

The problem is in emitInsSizeCV that overestimates the real instruction size and returns value > 15 that is the maximum decodable size for Intel instructions.
The fast and awful solution would be to change instrDesc::idCodeSize(unsigned sz) to shrink sz to 15 when it is bigger. Then codegen will know the real size and fail with its own assert if the real size is > 15.

The good solution would be to clean emitInsSizeCV and similar functions (emitInsSizeSV, emitInsSizeRR, emitInsSizeAM, emitInsSizeCV) but it looks costly, from the first look I was not able to extract any logical parts in that code (where do we calculate opcode size, where do we calculate prefixes, etc). The advantage of this approach would be that we will allocate less memory for code from VM in general.

[CarolEidt]

Here's the general issue for refactoring of the emitter, and the specific comment relating to instruction size: https://github.com/dotnet/coreclr/issues/23006#issuecomment-471114107

[sandreenko]

I have published PR with the first solution, meanwhile, I am working on a good one that uses emitOutputInstr to generate real bytes for the instr and get precise results.

It allows us to delete all functions that are doing estimates now and have very unclear code that we have to support every time when we change emitOutputInstr.

There are a few problems for far:

1. can't predict jump sizes, so have to assume the worst;
2. emitOutputInstr updates GC info that we need to skip when we estimate;
3. emitOutputInstr registers relocation that we also do not need during estimates;
4. a few methods in emitOutputInstr use predicated values from idCodeSize() to generate code (for nops for example);

2 and 3 are tricky because the code that does it is spread out through different places, but I am trying to extract it (for now I have template argument that I use to guard such places).

I think 2 weeks should be enough to get it into a decent state.

[mikedn]

I am working on a good one that uses emitOutputInstr to generate real bytes for the instr and get precise results.

That's interesting and scary at the same time - any idea what perf impact such an approach has?

[sandreenko]

That's interesting and scary at the same time - any idea what perf impact such an approach has?

In terms of throughput, the first iteration would be a bit slower, but emitOutputInstr is almost as expensive as our current version (when you skip the most expensive parts that do GC updates, that I think should not be there at all, but it is another issue) and communications with VM about relocs/static fields addresses.

The second iteration could memorize the generated bytes and keep them in Jit arena allocated memory, then memcpy it to VM memory and apply fixups. It could make TP better than it is now, but it is a far goal.

I do not expect any significant steady-state perf impact, we will allocate less memory for each method, so probably crossgen images could be smaller and we could have more short jumps, but It is hard to guess right now.

[mikedn]

but emitOutputInstr is almost as expensive as our current version (when you skip the most expensive parts that do GC updates, that I think should not be there at all, but it is another issue) and communications with VM about relocs/static fields addresses.

Right. And if the mechanism that skips GC updates, relocation & instruction byte emission is efficient (e.g. some template wizardry) the overhead could be very low, perhaps even 0. This sounds very tempting.

The second iteration could memorize the generated bytes and keep them in Jit arena allocated memory, then memcpy it to VM memory and apply fixups. It could make TP better than it is now, but it is a far goal.

Yeah, I was wondering why we simply don't encode instructions directly, there should be another way to deal with short/long jump than what the emitter does now.

[sandreenko]

Yeah, I was wondering why we simply don't encode instructions directly, there should be another way to deal with short/long jump than what the emitter does now.

because we do not have memory where to put them and do not know hot/cold block sizes for jumps. I think these estimates became a real problem when we started to add many new instructions (mostly for HW intrinsics) and because the code in estimates allowed to over-estimate it did not complain when the new predictions were very inaccurate.

Note: on arm32/arm64 we do not want to use a temporary Jit memory block to put instruction there and copy to VM after, we can estimate there simpler and better than on XARCH avoiding alloc/memcpy.

[AndyAyersMS]

@sandreenko do you think we should try and fix this for 5.0? How much overestimating is going on?

[sandreenko]

I don't remember the exact number, but it was very small, like 0.1%.

[kunalspathak]

There is a follow-up conversation at #8748 (comment) related to over-estimation of certain instructions.