Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid promoting over-sized intrinsic #27774

Merged

Conversation

@cshung
Copy link
Member

cshung commented Nov 8, 2019

This PR is meant to fix an assert in src\jit\morph.cpp:16898 when JIT\HardwareIntrinsics\X86\Avx\Avx_r\CPAOT-ret.out\Avx_r.dll is compiled under crossgen2

Here is my understanding on what is going on:

When we attempt to promote a struct, we obtain information about the struct though Compiler::StructPromotionHelper::GetFieldInfo, without the size check, that code believes Vector256<Double> is TYP_SIMD32 but it actually cannot be because we turned off Avx.

Then Compiler::StructPromotionHelper::TryPromoteStructField confused, it thinks Vector256<Double> is not a TYP_STRUCT and let the promotion happen.

Downstream, we failed, Compiler::fgMorphStructField discovered that we are promoting a struct field which is also a struct.

My confidence in this code change is just around 30%. It is very possible that something in crossgen2 is wrong instead.

@dotnet/crossgen-contrib

@janvorli

This comment has been minimized.

Copy link
Member

janvorli commented Nov 8, 2019

@tannergooding

This comment has been minimized.

Copy link
Member

tannergooding commented Nov 8, 2019

@cshung, could you share which method in Avx_r is causing the assert? I would suspect its one of the test.RunUnsupportedScenario scenarios since that is explicitly calling an unsupported method and checking for PNSE.

@cshung

This comment has been minimized.

Copy link
Member Author

cshung commented Nov 8, 2019

This is the function that when compiling causing the assertion (note that some functions are inlined):

Here is the field that got interpreted as TYP_SIMD32:

@CarolEidt

This comment has been minimized.

Copy link
Member

CarolEidt commented Nov 8, 2019

This will presumably change once we've settled the question of how to handle unsupported types - #23899 (comment) - but I think this is a reasonable fix. To summarize, this fix would not prevent getBaseTypeAndSizeOfSIMDType from returning TYP_SIMD32, but would prevent us from promoting a field of that type, since it would not fit into a register.

@tannergooding

This comment has been minimized.

Copy link
Member

tannergooding commented Nov 8, 2019

the question of how to handle unsupported types

I don't think that will change this here. That discussion (#23899 (comment)) was meant to be around interop handling (where both sides might have an inconsistent view of the world) not around the managed side (where we can always be consistent).

@CarolEidt

This comment has been minimized.

Copy link
Member

CarolEidt commented Nov 8, 2019

@tannergooding - my point was that, if we decide to outlaw unsupported types, then it would presumably be best to not even return TYP_SIMD32 in this case (i.e. change getBaseTypeAndSizeOfSIMDType()) . If, however, we want to support interop for some set of unsupported types (i.e. those for which we can still support ABI requirements even if we don't support intrinsic methods on them), then we do need to recognize them.

@cshung

This comment has been minimized.

Copy link
Member Author

cshung commented Nov 8, 2019

I am a little confused here. Is the change ready to go after adding the comment or we need further discussion? If this needs more discussion then I can try to arrange a [hopefully short] meeting.

@CarolEidt

This comment has been minimized.

Copy link
Member

CarolEidt commented Nov 8, 2019

Sorry @cshung - I guess I wasn't clear. I believe this is the right fix unless and until we decide to change how we handle unsupported types. It looks like it needs some formatting fixes but otherwise LGTM

@cshung

This comment has been minimized.

Copy link
Member Author

cshung commented Nov 8, 2019

@CarolEidt No worries, it was just me being uncertain about my fix. I will get the comment written and the formatting fixed.

@cshung cshung changed the title [WIP] Avoid promoting oversized intrinsic Avoid promoting over-sized intrinsic Nov 8, 2019
Copy link
Member

CarolEidt left a comment

LGTM thanks!

@cshung cshung merged commit 8e709de into dotnet:master Nov 9, 2019
50 checks passed
50 checks passed
WIP Ready for review
Details
coreclr-ci Build #20191108.33 had test failures
Details
coreclr-ci (Build Linux arm checked) Build Linux arm checked succeeded
Details
coreclr-ci (Build Linux arm64 checked) Build Linux arm64 checked succeeded
Details
coreclr-ci (Build Linux arm64 release) Build Linux arm64 release succeeded
Details
coreclr-ci (Build Linux x64 checked) Build Linux x64 checked succeeded
Details
coreclr-ci (Build Linux_musl x64 checked) Build Linux_musl x64 checked succeeded
Details
coreclr-ci (Build Linux_musl x64 release) Build Linux_musl x64 release succeeded
Details
coreclr-ci (Build Linux_rhel6 x64 release) Build Linux_rhel6 x64 release succeeded
Details
coreclr-ci (Build OSX x64 checked) Build OSX x64 checked succeeded
Details
coreclr-ci (Build OSX x64 release) Build OSX x64 release succeeded
Details
coreclr-ci (Build Test Pri0 Linux arm checked) Build Test Pri0 Linux arm checked succeeded
Details
coreclr-ci (Build Test Pri0 Linux arm64 checked) Build Test Pri0 Linux arm64 checked succeeded
Details
coreclr-ci (Build Test Pri0 OSX x64 checked) Build Test Pri0 OSX x64 checked succeeded
Details
coreclr-ci (Build Test Pri0 OSX x64 release) Build Test Pri0 OSX x64 release succeeded
Details
coreclr-ci (Build Test Pri0 Windows_NT arm checked) Build Test Pri0 Windows_NT arm checked succeeded
Details
coreclr-ci (Build Test Pri0 Windows_NT arm64 checked) Build Test Pri0 Windows_NT arm64 checked succeeded
Details
coreclr-ci (Build Test Pri0 Windows_NT x64 checked) Build Test Pri0 Windows_NT x64 checked succeeded
Details
coreclr-ci (Build Test Pri0 Windows_NT x86 checked) Build Test Pri0 Windows_NT x86 checked succeeded
Details
coreclr-ci (Build Windows_NT arm checked) Build Windows_NT arm checked succeeded
Details
coreclr-ci (Build Windows_NT arm release) Build Windows_NT arm release succeeded
Details
coreclr-ci (Build Windows_NT arm64 checked) Build Windows_NT arm64 checked succeeded
Details
coreclr-ci (Build Windows_NT arm64 release) Build Windows_NT arm64 release succeeded
Details
coreclr-ci (Build Windows_NT x64 checked) Build Windows_NT x64 checked succeeded
Details
coreclr-ci (Build Windows_NT x64 debug) Build Windows_NT x64 debug succeeded
Details
coreclr-ci (Build Windows_NT x64 release) Build Windows_NT x64 release succeeded
Details
coreclr-ci (Build Windows_NT x86 checked) Build Windows_NT x86 checked succeeded
Details
coreclr-ci (Build Windows_NT x86 debug) Build Windows_NT x86 debug succeeded
Details
coreclr-ci (Checkout (Unix)) Checkout (Unix) succeeded
Details
coreclr-ci (Checkout (Windows)) Checkout (Windows) succeeded
Details
coreclr-ci (Formatting Linux x64) Formatting Linux x64 succeeded
Details
coreclr-ci (Formatting Windows_NT x64) Formatting Windows_NT x64 succeeded
Details
coreclr-ci (Run Test Pri0 Linux arm checked) Run Test Pri0 Linux arm checked succeeded
Details
coreclr-ci (Run Test Pri0 Linux arm64 checked) Run Test Pri0 Linux arm64 checked succeeded
Details
coreclr-ci (Run Test Pri0 Linux x64 checked) Run Test Pri0 Linux x64 checked succeeded
Details
coreclr-ci (Run Test Pri0 Linux_musl x64 checked) Run Test Pri0 Linux_musl x64 checked succeeded
Details
coreclr-ci (Run Test Pri0 Linux_musl x64 release) Run Test Pri0 Linux_musl x64 release succeeded
Details
coreclr-ci (Run Test Pri0 OSX x64 checked) Run Test Pri0 OSX x64 checked succeeded
Details
coreclr-ci (Run Test Pri0 Windows_NT arm checked) Run Test Pri0 Windows_NT arm checked succeeded
Details
coreclr-ci (Run Test Pri0 Windows_NT arm64 checked) Run Test Pri0 Windows_NT arm64 checked succeeded
Details
coreclr-ci (Run Test Pri0 Windows_NT x64 checked) Run Test Pri0 Windows_NT x64 checked succeeded
Details
coreclr-ci (Run Test Pri0 Windows_NT x86 checked) Run Test Pri0 Windows_NT x86 checked succeeded
Details
coreclr-ci (Run Test Pri0 CoreFX Linux x64 checked) Run Test Pri0 CoreFX Linux x64 checked succeeded
Details
coreclr-ci (Run Test Pri0 CoreFX Windows_NT x64 checked) Run Test Pri0 CoreFX Windows_NT x64 checked succeeded
Details
coreclr-ci (Run Test Pri0 R2R Linux x64 checked) Run Test Pri0 R2R Linux x64 checked succeeded
Details
coreclr-ci (Run Test Pri0 R2R OSX x64 checked) Run Test Pri0 R2R OSX x64 checked succeeded
Details
coreclr-ci (Run Test Pri0 R2R Windows_NT x64 checked) Run Test Pri0 R2R Windows_NT x64 checked succeeded
Details
coreclr-ci (Run Test Pri0 R2R Windows_NT x86 checked) Run Test Pri0 R2R Windows_NT x86 checked succeeded
Details
coreclr-ci (Test crossgen-comparison Linux arm checked) Test crossgen-comparison Linux arm checked succeeded
Details
license/cla All CLA requirements met.
@cshung cshung deleted the cshung:dev/andrewau/avoid-promoting-oversized-intrinsic branch Nov 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.