Uh oh!
There was an error while loading. Please reload this page.
Enable JitDisasm in Release - #73365
Conversation
ghost
commented
Aug 4, 2022
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsThis PR enables It doesn't print lcl tables and gc info (for now). Adds ~30kb to the binary size of jit. The only noticeable impact on Release is that Example: publicclassTests{staticintstaticField;stringinstanceField;publicvirtualvoidVirtualCall(){}publicvoidTest(string[]args,IDisposabled){// control flowif(args?.Length==0){Console.WriteLine("field: "+staticField);Console.WriteLine(instanceField);}else{// SIMD (data section)Console.WriteLine(Vector128.Create(1,2,3,4));}// Virtual callVirtualCall();// Interface calld?.Dispose();// Jump tableswitch(args.Length){case1:Console.WriteLine("1");break;case2:Console.WriteLine("2");break;case3:Console.WriteLine("3");break;case4:Console.WriteLine("5");break;case6:Console.WriteLine("7");break;}Console.ReadKey();}}; Emitting BLENDED_CODE for X64 CPU with AVX - Windows; optimized code; rsp based frame; partially interruptible; No PGO data; 0 inlinees with PGO data; 2 single block inlinees; 0 inlinees without PGO dataIG_01:57pushrdi56pushrsi53pushrbx 4883EC30 subrsp,48 C5F877 vzeroupper 488BF1 movrsi,rcx 488BFA movrdi,rdx 498BD8 movrbx,r8IG_02: 4885FF testrdi,rdi 743D je SHORT G_M000_IG04IG_03: 837F0800 cmp dword ptr [rdi+8],07537jne SHORT G_M000_IG04 8B0DF8B40C00 movecx, dword ptr [(reloc 0x7ffd2d2dc06c)] FF15560A1100 call[Number:Int32ToDecStr(int):String] 488BD0 movrdx,rax 48B9D820C0EF74020000 movrcx,0x274EFC020D8 488B09 movrcx, gword ptr [rcx] FF15D0100900 call[String:Concat(String,String):String] 488BC8 movrcx,rax FF15FFAE0F00 call[Console:WriteLine(String)] 488B4E08 movrcx, gword ptr [rsi+8] FF15F5AE0F00 call[Console:WriteLine(String)] EB27 jmp SHORT G_M000_IG05IG_04: 48B978D62F2DFD7F0000 movrcx,0x7FFD2D2FD678 E8FCAEAF5F call CORINFO_HELP_NEWSFAST C5F91005E4000000 vmovupdxmm0, xmmword ptr [reloc @RWD00] C5F9114008 vmovupd xmmword ptr [rax+8],xmm0 488BC8 movrcx,rax FF15B6AE0F00 call[Console:WriteLine(Object)]IG_05: 488BCE movrcx,rsi 488B06 movrax, qword ptr [rsi] 488B4040 movrax, qword ptr [rax+64] FF5020 call[rax+32]Tests:VirtualCall():this 4885DB testrbx,rbx7410je SHORT G_M000_IG07IG_06: 488BCB movrcx,rbx 49BB0800062DFD7F0000 movr11,0x7FFD2D060008 41FF13 call[r11]IDisposable:Dispose():thisIG_07: 8B4F08 movecx, dword ptr [rdi+8] FFC9 dececx 83F905 cmpecx,5 0F877F000000 ja G_M000_IG13 8BC9 movecx,ecx 488D05AD000000 learax,[reloc @RWD16] 8B0488 moveax, dword ptr [rax+4*rcx] 488D1556FFFFFF leardx, G_M000_IG02 4803C2 addrax,rdx FFE0 jmpraxIG_08: 48B9E020C0EF74020000 movrcx,0x274EFC020E0 488B09 movrcx, gword ptr [rcx] FF1573AE0F00 call[Console:WriteLine(String)] EB52 jmp SHORT G_M000_IG13IG_09: 48B9E820C0EF74020000 movrcx,0x274EFC020E8 488B09 movrcx, gword ptr [rcx] FF155EAE0F00 call[Console:WriteLine(String)] EB3D jmp SHORT G_M000_IG13IG_10: 48B9F020C0EF74020000 movrcx,0x274EFC020F0 488B09 movrcx, gword ptr [rcx] FF1549AE0F00 call[Console:WriteLine(String)] EB28 jmp SHORT G_M000_IG13IG_11: 48B9F820C0EF74020000 movrcx,0x274EFC020F8 488B09 movrcx, gword ptr [rcx] FF1534AE0F00 call[Console:WriteLine(String)] EB13 jmp SHORT G_M000_IG13IG_12: 48B90021C0EF74020000 movrcx,0x274EFC02100 488B09 movrcx, gword ptr [rcx] FF151FAE0F00 call[Console:WriteLine(String)]IG_13: 488D4C2420 learcx, bword ptr 33D2 xoredx,edx FF158AEE0F00 call[ConsolePal:ReadKey(bool):ConsoleKeyInfo]90nopIG_14: 4883C430 addrsp,48 5B poprbx 5E poprsi 5F poprdi C3 retRWD00 dq 0000000200000001h,0000000400000003hRWD16 dd 000000AFh ; case IG_08dd000000C4h ; case IG_09dd000000D9h ; case IG_10dd000000EEh ; case IG_11dd00000116h ; case IG_13dd00000103h ; case IG_12; Total bytes of code 319
|
stephentoub
commented
Aug 4, 2022
This would be super helpful. |
Uh oh!
There was an error while loading. Please reload this page.
hez2010
commented
Aug 4, 2022
Does this PR also cover crossgen2? |
EgorBo
commented
Aug 4, 2022
Yes works for R2R. And, probably, for NativeAOT as well but afair it redirects stdout cc @MichalStrehovsky |
EgorBo
commented
Aug 4, 2022
One noticeable advantage over sharplab for end-users is the ability to print names for helper calls, e.g. publicclassC{objecto;publicvoidM(object[]a){a[0]=42;o=a;}}Sharplab:JitDisasm:; Assembly listing for method C:M(ref):this; Emitting BLENDED_CODE for X64 CPU with AVX - Windows; optimized code; rsp based frame; partially interruptible; No PGO dataG_M000_IG01: ;; offset=0000H57pushrdi56pushrsi 4883EC28 subrsp,40 488BF9 movrdi,rcx 488BF2 movrsi,rdxG_M000_IG02: ;; offset=000CH 48B958E4DC2CFD7F0000 movrcx,0x7FFD2CDCE458 E835AFB15F call CORINFO_HELP_NEWSFAST C740082A000000 mov dword ptr [rax+8],42 4C8BC0 movr8,rax 488BCE movrcx,rsi 33D2 xoredx,edx E891F4FFFF call CORINFO_HELP_ARRADDR_ST 488D4F08 learcx, bword ptr [rdi+8] 488BD6 movrdx,rsi E875F4E2FF call CORINFO_HELP_ASSIGN_REF90nopG_M000_IG03: ;; offset=003CH 4883C428 addrsp,40 5E poprsi 5F poprdi C3 ret; Total bytes of code 67Perhaps, it will be useful for sharplab too |
JulieLeeMSFT
commented
Aug 4, 2022
@EgorBo do you plan to merge this to .NET 7? |
EgorBo
commented
Aug 4, 2022
It probably does not meet the bar but I'd love to merge it in .NET 7.0, but no strong opinion |
FWIW, I would love that, too :) (as long as there's no meaningful perf regressions associated with it and there are no concerns about the data this exposes). |
EgorBo
commented
Aug 4, 2022
Yep, we discussed this one too and came to conclusion that there is nothing to be afraid of and our competitors provide this ability too |
jkotas
commented
Aug 4, 2022
NativeAOT compiler and crossgen use exact same infrastructure. If it works for one, it should work for the other one too. You have to specify the option on the command line instead of env variable. Is that correct?
We should not be injecting new life into I am wondering why we need both |
EgorBo
commented
Aug 4, 2022
Correct, e.g.
I fully agree, should I still keep NgenDisasm for backward compatibility? |
jkotas
commented
Aug 4, 2022
There is no backward compatibility to be worried about. We should delete all configs that start with If we have any infra that use the I see a few places that clear |
https://godbolt.org/ relies on it to print ASM for C#, but I assume it will only benefit from moving to release .NET as it won't have to compile .NET from sources and use public previews/daily builds of Release .NET instead cc @hez2010 |
Uh oh!
There was an error while loading. Please reload this page.
There is nothing to worry about. We can definitely setup a new net7.0 config for godbolt while keeping net6.0 as is. |
EgorBo
commented
Aug 8, 2022
Outerloop pipelines failures seem unrelated:
|
jakobbotsch
commented
Aug 8, 2022
I have started a non-PR run of The change is somewhat low risk for release since when My main concern would be that we have an |
EgorBo
commented
Aug 8, 2022
Right, I audited those too (and inserted a failfast if we entered them with .disAsm = false) locally |
EgorBo
commented
Aug 8, 2022
@dotnet/jit-contrib @jakobbotsch@BruceForstall PTAL |
AndyAyersMS
commented
Aug 8, 2022
Seems like we should allow users to specify If you are live debugging it would be nice to see the actual addresses and such. runtime/src/coreclr/jit/disasm.cpp Lines 1461 to 1467 in e71a958 Might also be nice to be able to control There are a whole bunch more like this: unwind, eh, ... but they might take a lot more work. |
omariom
commented
Aug 8, 2022
@EgorBo |
EgorBo
commented
Aug 8, 2022
Yes, I had to draw the line somewhere for the initial impl since, obviously, supporting all of them is a lot more work, I am going to take a look at |
Good question, I assume some 3rd party tools are going to depend on it (parse) and we should not ever change it |
There could be a separate env var that'd enable a special stable parsable output, something like git does for tools invoking it. |
There was a problem hiding this comment.
Looks good. It would be nice if the number of places doing + m_debugInfoSize were more minimized, via wrapper functions or the like.
It would also be nice if some of the additional controls were available, as previously suggested, like JitStdOutFile, JitDiffableDasm, etc.
| #endif // MULTIREG_HAS_SECOND_GC_RET | ||
| }; | ||
| // TODO-Cleanup: Uses of stack-allocated instrDescs should be refactored to be unnecessary. |
There was a problem hiding this comment.
Why should we eliminate stack-allocated instrDescs? It seems perfectly reasonable to allow them.
There was a problem hiding this comment.
We are creating these temporary instrDesc simply to be able to call existing methods to emit code/do printing. That seems like a code smell that the codegen/printing methods themselves should be refactored so that we can use the specific parts we want directly instead of having to indirect through an instrDesc structure. BTW, there's just three uses of this pattern:
- One in emitxarch.cpp that wants to call an emit method. It already has this same cleanup comment.
- One in emitarm.cpp and emitarm64.cpp for printing
LARGEJMPinstructions.
| { | ||
| private: | ||
| instrDescDebugInfo* idDebugInfo; | ||
| alignas(alignof(T)) char idStorage[sizeof(T)]; |
There was a problem hiding this comment.
Given the static_assert_no_msg below, isn't this alignas not useful? Or equivalent to alignas(sizeof(instrDescDebugInfo*))?
There was a problem hiding this comment.
In theory, some future instrDesc could have larger-than-pointer alignment requirement. That would certainly require some work to support and I just want it to be noisy that this is a place that would need to be fixed.
kunalspathak
commented
Aug 9, 2022
Started to review, but it seems we are not printing |
kunalspathak
commented
Aug 9, 2022
I agree with @BruceForstall . There are too many places where we are doing |
Consolidate most uses of m_debugInfoSize into these functions.
jakobbotsch
commented
Aug 9, 2022
I pushed a commit that adds |
Uh oh!
There was an error while loading. Please reload this page.
BruceForstall
commented
Aug 9, 2022
The functions are a nice improvement. |
jakobbotsch
commented
Aug 9, 2022
We get two new IAT entries on Windows: |
EgorBo
commented
Aug 10, 2022
Merging now. We're going to keep eyes on TP-related benchmarks (e.g. "Crossgen2 Throughput Trends" page) in coming weeks. Kudos to @jakobbotsch for mitigating all the initial TP/Memory regressions 🎉 |

This PR enables
DOTNET_JitDisasmfor Release config.It doesn't print lcl tables, EH and gc info (for now).
Adds ~30kb to the binary size of jit.
The only noticeable impact on Release is that
instrDescstruct is now 8bytes bigger (_idDebugOnlyInfo) - but I might fix it with a standalone hash-table which will be populated only when JitDisasm variable is not empty.Example:
DOTNET_JitDisasm=Test: (DOTNET_JitDiffableDasmis0by default, hence, raw bytes, addresses)