Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

JIT: fix arm64 crossgen simd issue #24071

Merged
merged 2 commits into from
Apr 18, 2019
Merged

Conversation

AndyAyersMS
Copy link
Member

Need to check supportSIMDType() and not featureSIMD in rationalize.

Fixes #24055.

Need to check `supportSIMDType()` and not `featureSIMD` in rationalize.

Fixes #24055.
@AndyAyersMS
Copy link
Member Author

@CarolEidt PTAL
cc @dotnet/jit-contrib

We should probably audit the other uses of featureSIMD...

@CarolEidt
Copy link

@AndyAyersMS - thanks! I'd started to look at this (@echesakovMSFT had mentioned this on #23675), but ran into some process issues.
Agreed that it would be good to audit the other uses of featureSIMD.

@CarolEidt
Copy link

@AndyAyersMS - there were also failures in RayTracer and Vector3Interop that were related to Vector3, and a few other testrs that seem to be SIMD-related that I haven't looked into: https://dev.azure.com/dnceng/public/_build/results?buildId=157586&view=ms.vss-test-web.build-test-results-tab

If you could verify that those succeed, that would be great. I don't think there's a way to trigger r2r testing on Arm64, but if you'd prefer I can check whether this fixes those as well.

@AndyAyersMS
Copy link
Member Author

If you could verify that those succeed, that would be great

Sure, I'll look into those failures. I already have everything set up and ready to go.

@AndyAyersMS
Copy link
Member Author

A bunch of these now pass, but some still fail.

DevDiv_714266 and GitHub_20260: fix looks similar.

Assert failure(PID 17282 [0x00004382], Thread: 17282 [0x4382]): Assertion failed 'comp->featureSIMD' in 'DevDiv_714266:MethodWithManyLiveVectors()' (IL size 200)

    File: /coreclr/src/jit/rationalize.cpp Line: 774
    Image: /home/andya/repos/coreclr/bin/tests/Linux.arm64.Checked/Tests/Core_Root/crossgen

ConsoleMandel: failure looks different.

Assert failure(PID 17347 [0x000043c3], Thread: 17347 [0x43c3]): Assertion failed '!"SIMD intrinsic with unsupported base type."' in 'Algorithms.VectorDoubleRenderer:RenderSingleThreadedWithADT(float,float,float,float,float):this' (IL size 440)

    File: /coreclr/src/jit/codegenarm64.cpp Line: 3878
    Image: /home/andya/repos/coreclr/bin/tests/Linux.arm64.Checked/Tests/Core_Root/crossgen

Let me verify the fix for the first two and then I'll look into this one.

@CarolEidt
Copy link

The ConsoleMandel one seems to be asserting on the UpperSave intrinsic. The gtSIMDBaseType is set based on the lclVar being saved, which in this case is created by CSE. It may be that its lvBaseType isn't being set.

@AndyAyersMS
Copy link
Member Author

Pushed a fix for the first assert.

@CarolEidt
Copy link

Thanks @AndyAyersMS - I think that the CSE case is another place where we're bitten by the fact that we don't retain handles on rvalues in all cases. I think the right thing to do here is probably a workaround, but I think something like @mikedn's work to cache the layout and retain it on rhs as well as lhs will be a better approach.

@AndyAyersMS
Copy link
Member Author

Workaround, as in ... ?

I may need to leave that part to you -- you can either take over this PR (add my public fork as a remote and push to the branch), or do that as a separate fix.

@CarolEidt
Copy link

The workaround would be to exempt the UpperSave and UpperRestore intrinsics from the base type check as the base type is not used in those.
Other intrinsics will have been created prior to CSE, and will have been set up with the proper base type.
Feel free to leave this one for me, and thanks for fixing the others!

@CarolEidt
Copy link

I'd suggest that we go ahead and merge your fixes, and I'll put up a separate PR to fix the ConsoleMandel case.

@AndyAyersMS
Copy link
Member Author

Formatter failure seems spurious.

@dotnet-bot retest Ubuntu x64 Formatting

@AndyAyersMS
Copy link
Member Author

Failed again -- ubuntu formatting leg seems to be broken:

17:29:36 Adding /mnt/j/workspace/dotnet_coreclr/master/x64_ubuntu_formatting_prtest/jitutils/bin to PATH
17:29:36 Done setting up!
17:29:36 Formatting jit directory.
17:29:36 Can't find compile_commands.json file. Running configure.
17:30:03 There was an error running CMake to generate compile_commands.json. Please run build.sh configureonly

@BruceForstall
Copy link
Member

There's an earlier error (from jitutils build.cmd?); not sure if that matters:

17:29:35 Required argument missing for option: --framework
17:29:35 Usage: dotnet publish [options] <PROJECT>

@AndyAyersMS
Copy link
Member Author

I think that particular failure is for the packages tool which isn't used here.

@AndyAyersMS
Copy link
Member Author

Arm64 failure was in the "contextual reflection" test. Seems likely to be unrelated? But did not fail in neighboring test runs. So will retry....

@dotnet-bot retest Windows_NT arm64 Cross Checked Innerloop Build and Test

@RussKeldorph
Copy link

@dotnet-bot test Ubuntu arm Cross Checked crossgen_comparison Build and Test

@RussKeldorph
Copy link

@dotnet-bot test Ubuntu x64 Formatting
@dotnet-bot test Ubuntu arm Cross Checked crossgen_comparison Build and Test

@CarolEidt
Copy link

@dotnet/jit-contrib - I propose that we go ahead and merge this. The only changes are under #ifdef FEATURE_SIMD, which is not enabled for arm, and that's the only test leg remaining.

@BruceForstall
Copy link
Member

@RussKeldorph Are you re-triggering crossgen_comparison because it failed? Because you're just adding to our arm32 backlog problem.

Copy link

@briansull briansull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks Good

@RussKeldorph
Copy link

@BruceForstall Yes.

@RussKeldorph RussKeldorph merged commit af83b8a into dotnet:master Apr 18, 2019
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
JIT: fix arm64 crossgen simd issue

Commit migrated from dotnet/coreclr@af83b8a
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test failure: JIT_SIMD._Plane_r_Plane_r_/_Plane_r_Plane_r_sh
5 participants