Update the `EmitterUnitTests` to support an alternative disassembler

[tannergooding]

Currently the emitter has a set of unit tests (genAmd64EmitterUnitTests and genArm64EmitterUnitTests) which can be used for validating that the emitted byte code is correct for various scenarios.

However, this does not currently work in CoreCLR itself due to a dependency on the closed source msvcdis disassembler.

Given the number of new instructions being supported and the number of general improvements being made to the emitter for the Hardware Intrinsics feature, it would be beneficial to update these tests such that they can be run as part of CoreCLR.

category:correctness theme:testing skill-level:intermediate cost:medium impact:small

[tannergooding]

@BruceForstall suggested that it might be possible to hook up the the CoreDisTools disassembly for this purpose (see https://github.com/dotnet/coreclr/pull/14572#issuecomment-337728416).

[tannergooding]

CC. @CarolEidt, @fiigii, @sdmaclea

[tannergooding]

This would also be beneficial for any refactoring that come out of https://github.com/dotnet/coreclr/issues/14523

[BruceForstall]

Note that SuperPMI already can use coredistools, and might provide a model: https://github.com/dotnet/coreclr/pull/13442.

Would be nice to hook up JitLateDisasm to coredistools, also.

[tannergooding]

I think I've discovered most of the things that need to be done.

Looks like we need to:

• Add a new altjit (or update an existing one) to specify -DLATE_DISASM=1
  • What is the recommendation here?
• Update src\jit\disasm to filter out the msvcdis specific methods (behind something like -DUSE_MSVCDIS)
• Update src\jit\disasm to reimplement DisAssembler::DisasmBuffer for CoreDisTools
  • The MSVCDIS looks to have some callback functionality which is not supported by CoreDisTools
  • What is the recommendation here?
• Create a new EmitterTests which
  • Sets COMPlus_AltJit
  • Sets COMPlus_AltJitName
  • Sets COMPlus_JitLateDisasm
  • Sets COMPlus_JitDump
  • The gen*EmitterUnitTests is currently dependent on this, having a different flag seems like it would be better
  • Runs an empty method through the JIT (possibly via crossgen, which would slightly change the variables listed above)
  • Ensures the regular disassembly (from JitDisasm) and the late disassembly are output to separate files
  • JITLateDisasmTo can be used to direct just the assembly to a file
  • There isn't a corresponding knob for JitDisasm, one could probably be added
  • Compares the regular disassembly to the late disassembly for accuracy

[BruceForstall]

Yes, we need USE_COREDISTOOLS and USE_MSVCDIS to distinguish those components. I think msvcdis supports a bunch of callbacks to get textual names for things that is not supported by coredistools. None of those are really required (I think) but do make the output easier to read. Maybe similar for labels.

On desktop, we built non-Release JITs with LATE_DISASM support enabled, but if you wanted it in Release, you needed to do a custom build. I would suggest this using coredistools for coreclr.

The overall goal here is to get the correct encodings emitted. We used the gen*EmitterUnitTests only for bring-up manual testing; we never actually had a real test using them. Is that really useful and worth building? Once you get the encodings right it would seem you wouldn't need to keep testing them. In particular, I think the "compare regular disassembly" (COMPlus_JitDisasm?) "to the late disassembly" function would be difficult to write, and possibly not worth it -- our JitDisasm format is pretty non-standard, and wouldn't be close to either coredistools or msvcdis.

[BruceForstall]

btw, @swaroop-sridhar might be able to help with the details of coredistools

[tannergooding]

The overall goal here is to get the correct encodings emitted. We used the gen*EmitterUnitTests only for bring-up manual testing; we never actually had a real test using them. Is that really useful and worth building?

In my opinion, yes.

Once you get the encodings right it would seem you wouldn't need to keep testing them.

The same could be said for most unit tests. Once you get the code right, it generally remains correct until the related production code is touched again.

Today, there isn't a "good" story for testing the JIT in general. That is, we don't have any kind of real native unit tests (outside of the PAL tests). Instead, we rely on a bunch of managed code being compiled down to the right thing for validation. This results in a lengthy turn-around for certain types of changes and doesn't give any kind of metric for how correct the code is, just that the current paths hit did the right thing.

A good example is the recent emitInsBinary refactoring I did. Due to our current setup, it requires running a bunch of manual tests for CoreCLR (and additional extended tests on Desktop, which can take 12+ hours to run and aren't open source) to validate that the code generated hasn't changed. This however still only gives a medium bar of confidence since we have no real way of determining what code paths were tested by the jitted code.

However, if we had proper unit testing of the JIT, we could have had extremely high confidence with a few unit tests that explicitly tested the various input cases (rather than requiring the current production JIT to hit those cases based on how the IL was read/processed/transformed/etc).

A unit test allows me to explicitly validate that INS reg, mem is emitted. The current setup requires that the JIT read the IL, transform it, mark the second node as contained, etc. The current setup also gives no real validation over whether the memory op was a static-address or a Scale,Index,Base triplet or if the VEX encoding emitted was 2-bytes or 3, etc. Unit tests make all of that trivial to validate.

This also isn't limited to just the emitter. It is just as accurate for any other part or phase of the JIT.

our JitDisasm format is pretty non-standard, and wouldn't be close to either coredistools or msvcdis.

This seems like it would be a good area to improve in general. There are a number of issues with the disassembly produced today that make it difficult to read and validate it is doing what you think.

[4creators]

In general I agree with @tannergooding that using native unit tests harness for the whole JIT (or just emitter) would allow for much faster and reliable tested development cycles.

[BruceForstall]

fyi @TIHan - related to disasm unit testing