Runtime: The "no." opcode prefix is not implemented

[ltrzesniewski]

ECMA-335 specifies the no. opcode prefix as the following (III.2.2):

This prefix indicates that the subsequent instruction need not perform the specified fault check
when it is executed. The byte that follows the instruction code indicates which checks can
optionally be skipped.

In short, it allows for automatic type checks, range checks or null checks to be skipped, which would be useful for optimizing hot code paths.

The current implementation throws InvalidProgramException whenever it encounters this prefix. This opcode is simply marked as unused right now:

https://github.com/dotnet/coreclr/blob/8499136a9a79fd37a4acb9dc690a4815edd8081d/src/inc/opcode.def#L320

Here's a simple method which reproduces the problem:

.method private hidebysig static 
    int32 NoPrefix () cil managed 
{
    .maxstack 2

    IL_0000: ldc.i4.1
    IL_0001: newarr [mscorlib]System.Int32
    IL_0006: ldc.i4.0
    IL_0007: no. 6 // this is rangecheck | nullcheck
    IL_000a: ldelem.i4
    IL_000b: ret
}

category:correctness
theme:msil
skill-level:intermediate
cost:medium

[tannergooding]

I would think that, at the very least, we should ignore the opcode (other than the "correctness/verifiability" checks) rather than throwing InvalidProgramException.

I do think that supporting the opcode would be beneficial though, as it is basically a built-in way of doing certain JIT hints (https://github.com/dotnet/corefx/issues/26188).

[tannergooding]

NOTE: Ignoring the prefix is allowed by the spec:

This prefix indicates that the subsequent instruction need not perform the specified fault check
when it is executed. The byte that follows the instruction code indicates which checks can
optionally be skipped. This instruction is not verifiable.

0x01: typecheck (castclass, unbox, ldelema, stelem, stelem). The CLI can optionally skip
any type checks normally performed as part of the execution of the subsequent instruction.
InvalidCastException can optionally still be thrown if the check would fail.

0x02: rangecheck (ldelem., ldelema, stelem.). The CLI can optionally skip any array range
checks normally performed as part of the execution of the subsequent instruction.
IndexOutOfRangeException can optionally still be thrown if the check would fail.

0x04: nullcheck (ldfld, stfld, callvirt, ldvirtftn, ldelem., stelem., ldelema). The CLI can
optionally skip any null-reference checks normally performed as part of the execution of the
subsequent instruction. NullReferenceException can optionally still be thrown if the check
would fail.

[sharwell]

I would think that, at the very least, we should ignore the opcode (other than the "correctness/verifiability" checks) rather than throwing InvalidProgramException.

This would also be a completely valid implementation of the instructions. Unlike the tail. prefix, the no. prefix specifies optional behavior.

[ltrzesniewski]

Yes, it's optional, and ignoring it would be the very least in order to satisfy the spec.

But I'd like to see it fully supported. I'm not familiar with the JIT implementation so I'm speculating here, but I'd guess that optionally removing existing checks shouldn't be too hard to implement.

[john-h-k]

If there is still use in this, I am currently implementing this so that the JIT just ignores it - during import, it just validates that the flags are correct and the opcode is correct and then does nothing with it. However, there are a couple of questions around it i am not sure about:

• Should no. 0x00 be invalid, or the same as not having it? (currently illegal to not have it
• Should we just read the bits of interest (0x1, 0x2, 0x4), or should we ensure no other bits are set?
• Should there be a PREFIX_NO added to prefixFlags that is set, but just currently ignored, or should it not do that and have no side effects other than necessary (codeAddr++ etc)?

[ltrzesniewski]

TBH implementing it so it's ignored is wasted effort unless it's a first step towards real support.

[john-h-k]

Implementing it so it is ignored would mean there was the potential for langs to expose it, even if it initially does nothing, which could make it worth it to actually make it omit the checks

[john-h-k]

Also, to be picky, helps ECMA compliance

[ltrzesniewski]

If future compatibility is what you want, I'd just ignore all invalid uses. This could be helpful if another flag is added to the instruction in the future. But that's only my opinion 😉

[john-h-k]

Currently have set it to only read bits it should be concerned with, so yeah, invalid and zero values are allowed. Am currently attempting to implement removal of range checks through GTF_INX_RNGHCK, so that will give it some actual meaning.

[john-h-k]

However, small issue so far :|
no. prefix is basically 3 seperate prefixes - typecheck, rangecheck, nullcheck, and they are all fundamentally different. I need to represent them in prefixFlags, which is across the board stored as int, pictured here:

The last 4 are added by me. Annoyingly, the third one, NULLCHK, overflows int data type, so either I need to drop support for one of the options or the enum needs to be promoted to __int64 or similar. This is my first time doing any contribution to the JIT so I have no clue whether that is ok or who to ask if it is, so any advice appreciated

[AndyAyersMS]

We should be able to represent 32 flags in an int, not 8. Can't you use a 0x2, 0x4, or 0x8 based value?

[ltrzesniewski]

Why is this written in hex in the first place? There must be a good reason as 0x100 is not the same as 1 << 2 and wastes space: you'd get room for 32 bits if you use 1 << 0, 1 << 1, 1 << 2 and so on...

[john-h-k]

I am appalled i did not notice a) it was in hex and b) if that was binary there would be values left over. Thanks for the help, but sorry for the appallingly stupid question 😂

[AndyAyersMS]

No worries.... what is there is misleadingly odd. We should double-check that there's nothing funny going with the uses that would somehow require those values.

[kindermannhubert]

This could be great opportunity for optimizing add-in for https://github.com/mono/linker
For example we can remove null checks based on C# 8.0 nonnull reference types. Prepend no. prefix before callvirt instruction. Do you think it would be possible?

Sorry for edits, I was writing faster than thinking.

[john-h-k]

Clearly a typo at the top there, it says stelem twice - i am making a guess it means stelem + stelem.*?

Also interesting rangecheck omittion is illegal for ldelem but not for ldelem.*?

[ltrzesniewski]

I'm pretty sure ldelem.* is supposed to include ldelem here, as the ldelem.* instructions are just shortcuts that compile to a smaller IL binary code.

The spec even says so:

All variants are equivalent to the ldelem instruction (§III.4.7) with an appropriate _typeTok_.

[AndyAyersMS]

Implementing it so it is ignored ...

Yes, a good first step would be to recognize and ignore the prefix. I would suggest being permissive in what you accept as any future extensions will likely have the "optional / hint" flavor to them too. So ignoring bits that are unspecified would be ok.

After that I would suggest working up a good set of test cases, and think through what should happen on them. Especially things like CSE candidates that differ only in no prefixes; we would want to make sure we handle the combinatorics properly (see for instance the recent work we have done for value numbering and exceptions: dotnet/coreclr#20129).

we can remove null checks based on C# 8.0 nonnull

Perhaps? It is not clear this is something the jit can rely on. And there are potentially other challenges: attribute recognition at jit time is expensive, and we may end up stripping this attribute away in implementation assemblies, at least in some cases.

[john-h-k]

I have a working ignoring version, I'll write up some tests that ensure the opcode following the prefix is valid and then can think up the test cases for actually implementing them

[john-h-k]

Writing up the "what is expected" for various situations where it is implemented as nop and when it is fully implemented. Mostly resolved, except whether no. 0x2 no. 0x1 should be allowed. Technically, it is multiple prefixes, and it could BADCODE like volatile.volatile. does, however, given this is internally treated as 3 separate prefixes PREFIX_NO_RANGECHK, PREFIX_NO_TYPECHK, PREFIX_NO_NULLCHK. Given how we are going for the lenient approach, only reading the bits we want, I am currently allowing it, provided there are no duplicate bits set

[AndyAyersMS]

That may be a bit too lenient? The spec is pretty clear that consecutive nos are invalid. Allowing any uint8 value after no is what I'd imagined you'd do for leniency.

Though I suppose even that might be questioned -- if some current IL provider sets some of the currently unused bits, then we might have unexpected problems if we decide to use those bits for something someday. So perhaps it's best to only accept the currently defined values?

[tannergooding]

I think that a single no. prefix and ignoring unrecognized values is the most correct/forward compatible.

It wouldn't be great if we wanted to start supporting some new prefix in the future (having it be optionally recognized as well) and then that library could no longer run on an older runtime. Especially if that was the only change to the library.

[john-h-k]

Switched it to fail on multiple prefixes now.

However, facing a bigger issue that is stumping me. It appears some sort of IL modification is happening before the reaching import to do with prefixes. It appears prefixes in invalid places are removed - I have 110% verified these prefixes are in the binary, by means of hexedit and ILDASM, but by the time it reaches compCompilerHelper these prefixes have been stripped. I tested a valid volatile. and valid unaligned. 2 prefix, and neither were removed, but an invalid volatile. call and invalid unaligned. 2 ldelem were both removed, alongside any no. prefixes. Really have zero clue what is happening here

[AndyAyersMS]

That seems odd -- I am not aware of anything in the runtime that would strip out prefixes -- editing IL on the fly is tricky. Can you verify the assembly being used at runtime is really the one you expect?

[john-h-k]

I am 100% sure it is - my commands are just:
ildasm HelloWorld.dll > dump.il ilasm dump.il /output=rcmp.exe %DEBUGCORERUN% rcmp.exe
and changes to the IL result in expected changes to the program.

[AndyAyersMS]

Perhaps you need to update this entry in opcode.def?

https://github.com/dotnet/coreclr/blob/5608b4ff0f81b99a5d436dec1e23b393503a4e07/src/inc/opcode.def#L320

[john-h-k]

I did, it is now
OPDEF(CEE_NO, "no.", Pop0, Push0, ShortInlineI, IPrefex, 2, 0xFE, 0x19, META)

It is then in fgImportBlockCode, I handle case CEE_NO:

[AndyAyersMS]

Ok ... what does %DEBUGCORERUN% point at in your command sequence above?

[john-h-k]

The path to the CoreRun, WindowsNt.x64.Debug or very close to that

[AndyAyersMS]

Depending on what you are doing there may be more than one copy of corerun in your tree. So which one are you using? is it under bin\Product or bin\Tests ?

[john-h-k]

Expected behaviour is observed with the program, an IndexOutOfRangeException is thrown when I perform an access past the end of the array I use, and not otherwise. It also prints what is expected when that exception is not thrown, and the IL to import when dumped is exactly as expected, except with these missing prefixes. I will link in a draft PR in a minute

[jkoritzinsky]

we can remove null checks based on C# 8.0 nonnull

Perhaps? It is not clear this is something the jit can rely on. And there are potentially other challenges: attribute recognition at jit time is expensive, and we may end up stripping this attribute away in implementation assemblies, at least in some cases.

I think the suggestion here with removing the null checks based on the C#8 feature was more directed at the idea of adding support to the IL Linker to add the .no prefix to the IL instructions based on the attributes. As long as that phase would run before the attribute-stripping phase (also would be part of the linker), then that should work without the JIT needing to have any concept of C#8 nullability, only knowledge of the .no prefix.

[john-h-k]

but looking at the diff on my draft PR, i am sure none of that code is responsible for the transformation.

[john-h-k]

Draft PR up at dotnet/coreclr#23851, has a couple of typos with variable names and there might be an erroneous bracket (fixed on local, will push when I next can), but that's not an issue atm because the code is never reached due to this weird disappearance of the prefix

[john-h-k]

@AndyAyersMS I used Product

[AndyAyersMS]

I'll take a look when I get a chance, might be a day or two.

[phdjonov]

Is this on the roadmap? I've done _awful_ things to trick the JIT into emitting the equivalent of a .no rangecheck|nullcheck ldelema in a project I maintain which I'd love to get rid of when compiling for netcoreapp3(.1?)+ targets.

[john-h-k]

This got lost, but I came back to it today and have a working version that supports and verifies all combinations of no. prefix. It has no tests tho, because I have no clue where/how to write JIT tests (that also need to be written in IL because it is not emitted by roslyn). It is spec compliant, as it is just a hint, but right now it has no actual functionality (it is verified and immediately discarded).

(From ECMA)

The byte that follows the instruction code indicates which checks can
optionally be skipped.

[am11]

See https://github.com/dotnet/runtime/blob/b93693ed830a944dde64138d305e3506bcaea911/src/coreclr/tests/src/JIT/IL_Conformance/Old/Conformance_Base/

Not sure if Old in its path means obsolete, but ckfinite ILOpcode is also not emitted by roslyn and tested directly via IL.

[john-h-k]

Awesome, I'll start on that. In the meantime, worth opening a draft PR to show the diff?