Conversation
src/inc/corjit.h
Outdated
There was a problem hiding this comment.
Thanks to Pat who will expand more Jit flags, I use this unused flag, instead.
There was a problem hiding this comment.
why doesn't this just replace the CORJIT_FLG_UNUSED7 definition above, such that the 0x00004000 values are adjacent?
|
@dotnet/jit-contrib PTAL. The corresponding encoder for CFI is in LLILC repo -- dotnet/llilc#1011 |
src/jit/unwindamd64.cpp
Outdated
There was a problem hiding this comment.
Are these XMM encodings correct?
There was a problem hiding this comment.
I validated this CFI assembly directive. Dwarf XMM0 starts with 17 instead of REG_XMM0 (16). Note actually since all xmm are volatile in Unix ABI, we don't save them at all so this part is actually not used, but leave it for consistency.
There was a problem hiding this comment.
Nit: why not just use a static table of mappings? If that's not possible, then I think I'd prefer that each case simply returned rather than mutating a local and leaving the return to the end. Something like:
assert(reg >= 0 && reg <= REG_XMM15);
switch (reg)
{
case REG_RAX: return 0;
case REG_RCX: return 2;
case REG_RDX: return 1;
case REG_RBX: return 3;
case REG_RSP: return 7;
case REG_RBP: return 6;
case REG_RSI: return 4;
case REG_RDI: return 5;
default:
return reg < REG_XMM0 ? reg : reg + 1; // shift by 1 due to dwarf_reg_ip
}
|
I don't like how this looks. You're creating both CFI and Windows unwind info all the time, but only using one. Why don't you have a completely separate implementation of the unwind APIs which compiled for CFI generation? That is, one unwindAllocStack for Windows, one for CFI. Do you need to choose between these dynamically? If so, then have the AMD64 unwindAllocStack() dynamically choose between unwindAllocStackWindows() and unwindAllocStackUnix() (for example). Sounds like a good use of function pointers or virtual functions. |
src/inc/clrnt.h
Outdated
There was a problem hiding this comment.
It would be better for this structure to live in separate file, maybe that is local to the JIT.
There was a problem hiding this comment.
Makes sense. Probably instead of copying OpType directly from LLVM, I may introduce my own enum just for 3 cases for now. This way we can avoid license concern as well as don't need to worry about syncing this definition when LLVM source is changed.
|
@BruceForstall Both CFI and Windows unwind code is not completely orthogonal. Windows unwind code/emission sequence is used to emit epilogue, etc. So, I'd like to keep Window's unwind code process intact. Here CFI emission is not complete blob in the sense it's not usual until it is laid out by ObjectWriter/LLVM. So, think this CFI emission is a code-generation mode (under CoreRT) which tides to Window's code sequence,
I am also not a big fan of virtual function stuff, and I don't see much value on this simple case. |
I'm not sure what you mean by this. It looks like consistent code generation should be ensured by the unwind info interface (i.e. the various
I'm not sure that the former question has been answered yet. This really boils down to "do we want to be able to build RyuJIT s.t. a single build may generate code for multiple target platforms". AFAIK this is not possible today, and I'd be hesitant to make significant moves in that direction without strong justification (e.g. easier cross compilation). Just to be clear, I am not advocating that we further bifurcate the JIT/EE interface using #ifdefs (which we unfortunately have already done): I'm just suggesting that we think hard before doing the opposite to the JIT implementation and measure afterwards. In terms of the overall direction: I thought that we were intending to introduce our own CFI codes that were compatible with what is expressible in DWARF and Windows unwind information s.t. we could have RyuJIT emit format-agnostic codes. Is this still the plan? If so, did it just so happen that DWARF CFI is expressive enough to cover this design? |
|
I talked to @kyulee1 in person; he can elaborate what he'll do (or, just make the changes and update the PR). @pgavlin: I'm not in the loop for CoreRT, but I thought we had no plans to change how we generate Windows unwind codes. non-Windows codes could be pseudo-codes that work broadly on non-Windows platforms. Also, it turns out for non-Windows, CoreCLR (but not CoreRT), we do need to generate Windows unwind codes. And a desire for single RyuJIT binary that works for CoreCLR or CoreRT per-platform then requires dynamically generating one form or the other. |
That's certainly correct when targeting CoreCLR et. al. I think this is still up in the air for CoreRT, however.
Yes, that's correct.
That's what I was missing! Thanks. |
|
@BruceForstall As discussed, I updated PR.
There are some disruptiveness and code duplication due to splitting functions (for Windows/CFI), but I think they are reasonably small. |
kyulee1
force-pushed
the
cfi
branch
3 times, most recently
from
b3c60d9 to
acc3f57
Compare
src/inc/cfi.h
Outdated
There was a problem hiding this comment.
Stephen Toub changed all the header file license agreements yesterday, so you should update this to match the new standard.
src/jit/compiler.h
Outdated
There was a problem hiding this comment.
coding convention: * is placed adjacent to type. https://github.com/dotnet/coreclr/blob/master/Documentation/coding-guidelines/clr-jit-coding-conventions.md#10.1
|
I like the change. Modulo the few comments, it looks good to me. |
There was a problem hiding this comment.
Do we have any estimate on how many CFI codes we'll typically need for a function? If so, can we give this vector a reasonable initial capacity?
There was a problem hiding this comment.
I'm not sure what the minimal bar. Many small functions need very small CFIs like 1 ~2. But complex functions with lots of eh could need many. @BruceForstall Can you suggest what the reasonable size for this vector?
There was a problem hiding this comment.
Not sure. Assuming it is 1-to-1 CFI-to-Windows unwind codes, you could probably do a pretty simple textual analysis of the generated ngen-based asm diffs files to figure this out.
|
LGTM modulo the comments above. |
For Unix targeting CoreRT, this will provide platform specific unwind info which is a dwarf format. Unlike window’s UNWIND_INFO, we won’t encode the format within RyuJit since it is not only complex/error-prone but also needs to be adjusted depending on platforms. Instead, CFI (call frame information) pseudo instructions are encoded, which will be passed to CoreRT/ObjectWriter which will translate them to directly emit CFI directives to LLVM MC stream to establish frames and layout eh_frame section. A jit flag is used to dynamically dispatch either Windows’s unwind blob or this CFI instruction table. No JIT/EE interface change is needed since the API already expects an opaque blob. Initially when I looked at what Clang does, the prologue and the sequence of CFI emissions are a bit different than Windows. Since we will emit the same sequence of code with the same runtime that we define, I assume this is unnecessary – I’ve verified the CFI sequence here can work correctly with libunwind. Basically we need only 3 operations – push reg is a combination of 1 and 2 below. 1. Allocation – increase stack frame. Normally subtract esp in x64. 2. Store – Copy a (callee save) register to stack (memory) 3. SaveFP – Set frame pointer register Since Windows operation is based on the relative value (all offsets, etc are computed from the current point), I also use the similar form of CFI instructions. So, mostly one window’s code corresponds to two CFIs – we might optimize this by aggregating allocation, but I’d like to keep the current syntax/semantic same as Windows. This results in a very simple transformation on par with Windows.
kyulee1
force-pushed
the
cfi
branch
2 times, most recently
from
acc3f57 to
b5f2fda
Compare
|
@BruceForstall @pgavlin Updated the change per feedbacks.
|
|
LGTM. Thanks for doing this! |
|
LGTM |
|
Failure seems unrelated. |
7 participants
For Unix targeting CoreRT, this will provide platform specific unwind
info which is a dwarf format.
Unlike window’s UNWIND_INFO, we won’t encode the format within RyuJit
which is not only complex/error-prone but also will be slightly
different depending on platforms.
Instead, CFI pseudo instructions are encoded, which will be passed to
CoreRT/ObjectWriter whi will translate them to directly emit CFI
instructions in LLVM MC to establish frames.
Then LLVM MC will handle the details of laying out the eh_frame
sections.
One a JitFlag is used to dynamically dispatch either Windows’s unwind
blob or this CFI instruction table. No JIT/EE interface change is needed
since we’ve already passing an opaque blob.
Initially when I looked at what Clang does, the prologue and the
sequence of CFI emissions are a bit different than Windows.
Since we will emit the same sequence of code with the same runtime that
we define, I assume this is unnecessary – I’ve verified the CFI sequence
here can work correctly with libunwind.
Basically we need only 3 operations – push reg is a combination of 1 and
2 below.
Since Windows operation is based on the relative value (all offsets,
etc are computed from the current point), I also use the similar form of
CFI instructions .
So, mostly one window’s code corresponds to two CFIs – we might optimize
this by aggregating allocation, but I’d like to keep the current
syntax/semantic same as Windows.
This results in a very simple transformation in par with Windows.