Check opportunistic ISAs in crossgen for CompExactlyDependsOn #90326
Conversation
…ledIntoFinalImage
ghost
assigned 
|
@dotnet/crossgen-contrib PTAL |
| @@ -604,7 +604,9 @@ public static bool ShouldCodeNotBeCompiledIntoFinalImage(InstructionSetSupport i | |||
| // This instruction set isn't supported on the current platform at all. | |||
| continue; | |||
| } | |||
| if (instructionSetSupport.IsInstructionSetSupported(instructionSet) || instructionSetSupport.IsInstructionSetExplicitlyUnsupported(instructionSet)) | |||
| if (instructionSetSupport.IsInstructionSetSupported(instructionSet) || | |||
| instructionSetSupport.IsInstructionSetOptimisticallySupported(instructionSet) || | |||
There was a problem hiding this comment.
The comment above says "and none of the intrinsic types are in an opportunistic state", so the current behavior looks very intentional and not a bug.
Maybe trigger some optional crossgen2 test runs to see whether the test will identify the problem with this change?
There was a problem hiding this comment.
A minimal repro to illustrate the problem:
void Foo()
{
if (Ssse3.IsSupported)
{
Bar();
}
}
[CompExactlyDependsOn(typeof(Ssse3))]
void Bar()
{
var a = Ssse3.Shuffle(..);
...
}The current behavior for crossgen2 (exactIsaLevel=Sse2, opportunisticIsaLevel=Sse4.2) is to ignore Bar(). But at the same time it's going to compile Foo as:
void Foo()
{
if (runtimeSsse3Check == true)
{
Bar(); // will trigger Tier0 jit compilation
}
}So feels like we do need to prejit methods demanding opportunistic ISAs. Another option is to lift [CompExactlyDependsOn(typeof(Ssse3))] to Sse2 but that won't work - analyzer will complain on missing Ssse3 check and also, analyzer is not aware about our opportunistic ISA level so this has to be done in crossgen instead.
cc @davidwrighton who introduced this attribute and the analyzer in #85481
There was a problem hiding this comment.
As far as I remember we can't use Avx as opportunistic with Sse as exact so presumably we can't hit Vector<> mismatch problems
|
/azp list |
This comment was marked as resolved.
This comment was marked as resolved.
|
/azp run runtime-coreclr outerloop, runtime-coreclr jitstress-isas-x86, runtime-coreclr crossgen2 |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
The Can this change have bad interactions with inlining? Consider the following pattern: If we inline |
|
@sbomer Could you please take a look at this one? |
My understanding that |
If it was ok to assume this, it should not be necessary to block inlining of |
|
My understanding of the original problem is that we can't pre-jit methods that assume an unsupported instruction set because the use of unsupported instructions would be precompiled to throw. If such a method is reached at runtime, we're in trouble. I think it would be ok to inline such a method as long as the caller's If we allowed inlining of such helpers, we would have also had to block prejit of the callers, up until a matching For the opportunistic instruction set, I don't actually see a reason why we couldn't prejit such helpers. If we produce non-throwing code for |
Yep, in this PR only lifts the limitation for opportunistic ISAs, so none of instructions are expected to turn into Throw()
I wasn't aware we block them. Do you ask because otherwise |
I didn't realize we block inlining either until @jkotas pointed it out. This check here: |
|
So I understand the problem but am still struggling to imagine a case where |
Yes, and also to help me to build a model around how this is meant to work. I believe that we have the tools to precompile everything and drop this filtering. For example, if I have method marked with |
|
My (limited) understanding last time I talked to @davidwrighton was that prejitted Avx2 helpers will throw, because crossgen assumes that Avx2 is unavailable in prejitted code. If we treated Avx2 the same as the optimistic instruction set in this PR, then I agree, I don't know why we wouldn't be able to prejit them. The current behavior might be a tradeoff that reduces prejitted code size. |
|
I wasn't there when these things were added so I can only guess: For crossgen I can understand why we should not prejit a method that e.g. assumes AVX is there, e.g.: void Foo()
{
if (Avx.IsSupported)
{
Bar();
}
}
[CompExactlyDependsOn(typeof(Avx))]
void Bar()
{
Avx.MoveMask();
}Let's say we prejit both. Then, we re-jit Foo to tier1 and this time we take the AVX path as it's actually supported but don't inline I assume you might wonder why JIT just doesn't emit AVX instructions as demanded even if it's asked to emit SSE2 level of code - I personally don't have an answer on why we don't do it today in JIT. I can only guess that it might be something to do with mixing different encodings: legacy/VEX/EVEX (perhaps, someone from @dotnet/jit-contrib knows more?) And, back to the topic, I can't imagine a similar scenario for opportunistic ISAs (as they're quite limitted - e.g. we can't have exact SSE and opportunistic AVX) |
|
Another (theoretical) problem that we might have even today is if we e.g. initialize some static (readonly) field from R2R assuming AVX is not available and e.g. create an array of 4 floats because of that. But then, after rejit we assume the array was created with AVX in mind, hence, the array has 8 floats (because there it is AVX, available now) and go out of bounds. I kind of feared |
You would have to track the allowed encodings per basic block and optimizations like CSE would have to be aware of the allowed encoding - to avoid moving code that requires the more advanced encoding into a place where the more advanced encoding is not allowed. I can believe that this is hard and fragile. |
|
Re: |
|
Although, I feel like opportunistic static void Main()
{
Test();
}
[MethodImpl(MethodImplOptions.NoInlining)]
static void Test()
{
if (Sse41.IsSupported || AdvSimd.Arm64.IsSupported)
Console.WriteLine("SSE41 or NEON");
else
Console.WriteLine(":(");
}NativeAOT: ; Assembly listing for method Prog:Test() (FullOpts)
; Emitting BLENDED_CODE for generic X64 - Windows
; FullOpts code
; NativeAOT compilation
sub rsp, 40
test byte ptr [(reloc 0x4000000000421990)], 16 ; data for <Module>:g_cpuFeatures
je SHORT G_M14333_IG05
lea rcx, gword ptr [(reloc 0x4000000000421998)] ; '"SSE41 or NEON"'
call System.Console:WriteLine(System.String)
nop
add rsp, 40
ret
G_M14333_IG05: ;; offset=0x001F
lea rcx, gword ptr [(reloc 0x40000000004219a0)] ; '":("'
call System.Console:WriteLine(System.String)
nop
add rsp, 40
ret ^ it works on NAOT. Crossgen: ; Assembly listing for method Prog:Test() (FullOpts)
; Emitting BLENDED_CODE for generic X64 - Windows
; FullOpts code
; ReadyToRun compilation
sub rsp, 40
mov rcx, qword ptr [(reloc 0x40000000004200b0)] ; const ptr
mov rcx, gword ptr [rcx]
call [System.Console:WriteLine(System.String)]
nop
add rsp, 40
ret
Maybe that's why |
The opportunistic checks are emitted as method fixups in R2R. In r2r dump, you can see things like Notice that |
|
@jkotas got it. Does it make sense to also make |
|
I do not think that it is worth it for R2R. It is ok for R2R to depend on fallback to JIT. Porting this code would not fix all cases of fallback to JIT. We would still need to fallback to JIT for methods with the legacy/VEX/EVEX problem. |
On NAOT, IsSupported is only emitted for opportunistic ISA checks so no encoding problems. E.g. with the default settings, |
The difference between corelib and non-corelib code is that the unsupported instruction set fixups are omitted for CoreLib. We assume that the CoreLib code is going to behave correctly even when |
|
@jkotas r2rdump (with this change) for |
Yes, it does pick up the fixup by some other path. The The current spec for the attribute says this:
This spec change is safe and it should not introduce observable functional behavior changes. The implementation needs to match the spec and it should not depend on the function to pick up the fixup by some other path. If you would like to change the spec for the attribute to something else, it is fine too - needs to be written down and we need to review and adjust the system to conform with the spec. BTW: There is a dead duplicate definition of the attribute in https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/CompExactlyDependsOn.cs . You can delete this file while you are on it. |
|
Hmm, I have been thinking about it as "the IsSupported property value must be known for the given method at AOT time". Well-defined only means that the code at runtime should not be able to observe different values of IsSupported property at different times. So the question is whether it is possible for a wrong value of IsSupported to sneak in somehow with the current change. A potential bug would most likely show up as invalid instruction fault on low-end hardware that does not support the opportunistic instruction set. Do we have any test running on low-end hardware like that? |
We have plenty of tests that stress downlevel ISAs, but none that explicitly try to fault AFAIK. Just ones that ensure the JIT throws a PNSE instead. I don't believe we have any hardware this old on which to test and expect we won't find such a machine outside of Linux. Chromium (and therefore Chrome and potentially Edge) has required SSE3 since 2014 and the ISA has been around on both AMD/Intel since ~2005. x64 MacOS likewise defaults to So if we wanted to try this we might be able to explicitly try around SSE4.1 (which doesn't require the VEX/EVEX encoding). |
Thanks for the hints! I tried to make [ExactlyDependsOn] to report InstructionSet via EgorBo@9f1c502 and it worked (tested on an empty method with |
It should be fine to check the custom attributes before compiling every method. |
|
I'm back from my trip, and reading through this thread, there are a couple of points...
Finally, we can't get rid of all of the filtering as we absolutely have to filter out instruction sets which happen to be disabled (such as Avx2, or Avx512) as otherwise we will generate incorrect code for the methods while building corelib. |
|
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
@stephentoub noticed that the following method shows up as non-prejitted in an empty app:
Crossgen uses SSE2 as a baseline (+ SSE4.2 as opportunistic) we should still compile this method since it can be reached with opportunistic ISAs.
With this fix I no longer see this method non-prejitted (along with a few others)