Skip to content

[JIT] ARM64 - Do not allow move and shifting with MSL on 16-bit vectors#77123

Merged
TIHan merged 2 commits intodotnet:mainfrom
TIHan:arm64-msl-16bit-fix
Oct 19, 2022
Merged

[JIT] ARM64 - Do not allow move and shifting with MSL on 16-bit vectors#77123
TIHan merged 2 commits intodotnet:mainfrom
TIHan:arm64-msl-16bit-fix

Conversation

@TIHan
Copy link
Contributor

@TIHan TIHan commented Oct 17, 2022 •
edited
Loading

Decription

Should resolve: #76880

We were allowing the movi instruction in conjunction with MSL to be emitted on 16-bit vectors which is not supported; it is only supported for 32-bit vectors. See https://developer.arm.com/documentation/ddi0596/2021-12/SIMD-FP-Instructions/MOVI--Move-Immediate--vector--?lang=en

Acceptance Criteria

  • JIT Stress run passes

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 17, 2022
@ghost ghost assigned TIHan Oct 17, 2022
@ghost
Copy link

ghost commented Oct 17, 2022

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Decription

Should resolve: #76880

We were allowing the movi instruction in conjunction with MSL to be emitted on 16-bit vectors which is not supported; it is only supported for 32-bit vectors. See https://developer.arm.com/documentation/ddi0596/2021-12/SIMD-FP-Instructions/MOVI--Move-Immediate--vector--?lang=en

Acceptance Criteria

  • JIT Stress run passes
Author: TIHan
Assignees: -
Labels:

area-CodeGen-coreclr
Milestone: -

@TIHan TIHan mentioned this pull request Oct 17, 2022
Closed
@AndyAyersMS
Copy link
Member

AndyAyersMS commented Oct 17, 2022

Cool, thanks! What code do we generate instead?

@TIHan
Copy link
Contributor Author

TIHan commented Oct 17, 2022

We generate:

mvni    v16.8h, #0

Which basically does an inverse of the bits if I'm reading this correctly: https://developer.arm.com/documentation/ddi0596/2021-12/SIMD-FP-Instructions/MVNI--Move-inverted-Immediate--vector--?lang=en

This is determined in the emitter from this logic:

                // Next try the ones-complement form of the 'immediate' imm(i8,bySh)
                if ((elemsize == EA_2BYTE) || (elemsize == EA_4BYTE)) // Only EA_2BYTE or EA_4BYTE forms
                {
                    notOfImm  = NOT_helper(imm, getBitWidth(elemsize));
                    canEncode = canEncodeByteShiftedImm(notOfImm, elemsize, true, &bsi);
                    if (canEncode)
                    {
                        imm = bsi.immBSVal;
                        ins = INS_mvni; // uses a mvni encoding
                        assert(isValidImmBSVal(imm, size));
                        fmt = IF_DV_1B;
                        break;
                    }
                }

@AndyAyersMS
Copy link
Member

AndyAyersMS commented Oct 17, 2022

Wonder if something bad happened to our azure storage:

call powershell Invoke-WebRequest -Uri "[https://clrjit.blob.core.windows.net/clang-tools/windows/clang-format.exe"](https://clrjit.blob.core.windows.net/clang-tools/windows/clang-format.exe%22) -OutFile bin\clang-format.exe
Invoke-WebRequest : ResourceNotFoundThe specified resource does not exist.

Similar problems in the SPMI legs.

@BruceForstall any ideas?

@BruceForstall
Copy link
Contributor

BruceForstall commented Oct 17, 2022

The Azure Storage issue should be fixed now. Please re-run the failing jobs.

@AndyAyersMS
Copy link
Member

AndyAyersMS commented Oct 17, 2022

@TIHan can this happen in .NET 7 or is this something new? Wondering if we might need to backport.

@TIHan
Copy link
Contributor Author

TIHan commented Oct 17, 2022

@AndyAyersMS It looks like it could happen in .NET 7. Some of the code surrounding this hasn't changed since 2015.

@AndyAyersMS
Copy link
Member

AndyAyersMS commented Oct 18, 2022 •
edited
Loading

The simple repro you created in #76880 works ok on .NET 7 RC1, so perhaps this is a recent change? Also, the test that fails here probably hasn't been failing for long. Might be good to pin down when our codegen changed.

; Assembly listing for method X:Main(<unnamed>):int
; Emitting BLENDED_CODE for generic ARM64 CPU - Windows
; MinOpts code
; fp based frame
; partially interruptible

G_M000_IG01:                ;; offset=0000H
        A9BD7BFD          stp     fp, lr, [sp,#-0x30]!
        910003FD          mov     fp, sp
        F90017A0          str     x0, [fp,#0x28]

G_M000_IG02:                ;; offset=000CH
        4F000410          movi    v16.4s, #0x00
        3D8007B0          str     q16, [fp,#0x10]
        529FFFE1          mov     w1, #0xFFFF
        320083E0          mov     w0, #0x10001
        1B007C21          mul     w1, w1, w0
        4E040C30          dup     v16.4s, w1
        4EB01E10          mov     v16.16b, v16.16b
        3D8007B0          str     q16, [fp,#0x10]
        910043A1          add     x1, fp, #16
        D29D1900          movz    x0, #0xE8C8
        F2B41060          movk    x0, #0xA083 LSL #16
        F2CFFFA0          movk    x0, #0x7FFD LSL #32
        97FFFC1D          bl      CORINFO_HELP_BOX
        D2935601          movz    x1, #0x9AB0
        F2B410E1          movk    x1, #0xA087 LSL #16
        F2CFFFA1          movk    x1, #0x7FFD LSL #32
        F9400021          ldr     x1, [x1]
        D63F0020          blr     x1
        2A1F03E0          mov     w0, wzr

G_M000_IG03:                ;; offset=0058H
        A8C37BFD          ldp     fp, lr, [sp],#0x30
        D65F03C0          ret     lr

; Total bytes of code 96

<65535, 65535, 65535, 65535, 65535, 65535, 65535, 65535>

[Edit: changed to show minopts codegen]

@TIHan
Copy link
Contributor Author

TIHan commented Oct 18, 2022

The disasm you posted is optimized. I think it only emits movi when it is not.

@AndyAyersMS
Copy link
Member

AndyAyersMS commented Oct 18, 2022

The disasm you posted is optimized. I think it only emits movi when it is not.

True -- fixed it above. Still doesn't have the bug.

@TIHan
Copy link
Contributor Author

TIHan commented Oct 18, 2022

I'll try to investigate when this started occurring then.

@AndyAyersMS
Copy link
Member

AndyAyersMS commented Oct 18, 2022

@dotnet/jit-contrib any way to get past this without starting from scratch?

Artifact SuperPMI_Asmdiffs_x86_checked already exists for build 55025.

@BruceForstall
Copy link
Contributor

BruceForstall commented Oct 18, 2022

@dotnet/jit-contrib any way to get past this without starting from scratch?

Artifact SuperPMI_Asmdiffs_x86_checked already exists for build 55025.

You can ignore it. This happens we using "Rerun" to re-run a portion of a job. We use the AzDO PublishPipelineArtifact@1 task to upload log files, and it doesn't allow overwriting previously uploaded log files. Most of the YAML files in dotnet/runtime use the continueOnError: true option to simply ignore any failures. The JIT ones (currently) don't do this, so we don't ignore the failures, but we also use condition: always() on all subsequent tasks so we don't stop when hitting this failure, either. Even if we did use continueOnError: true, a rerun still wouldn't overwrite the old log file, which seems like odd behavior.

There is some discussion on this here, for example: https://github.com/MicrosoftDocs/azure-devops-docs/issues/10543

@markples
Copy link
Contributor

markples commented Oct 18, 2022 •
edited
Loading

For logs, using $(System.JobAttempt) is probably a good fit. However, I don't understand why the docs say "Available in templates? -- no", so it might take some variable shenanigans to get the value in.

@AndyAyersMS
Copy link
Member

AndyAyersMS commented Oct 18, 2022

Just want to know if this PR had any diffs. It looks like it doesn't. @TIHan did you do a local SPMI diff run?

@TIHan
Copy link
Contributor Author

TIHan commented Oct 18, 2022

I'll run a local SuperPMI diff to see what we get.

@TIHan
Copy link
Contributor Author

TIHan commented Oct 18, 2022

There are no SuperPMI diffs found:

[12:23:39] ================ Logging to c:\work\runtime\artifacts\spmi\superpmi.6.log
[12:23:39] Using JIT/EE Version from jiteeversionguid.h: 982ed1fd-7bf3-425b-8b8a-902873151e79
[12:23:39] Using coredistools found at c:\work\runtime\artifacts\tests\coreclr\windows.x64.Checked\Tests\Core_Root\coredistools.dll
[12:23:39] Found download cache directory "c:\work\runtime\artifacts\spmi\mch\982ed1fd-7bf3-425b-8b8a-902873151e79.windows.arm64" and --force_download not set; skipping download
[12:23:39] SuperPMI ASM diffs
[12:23:39] Base JIT Path: C:\work\sandbox\runtime\artifacts\bin\coreclr\windows.x64.Checked\clrjit_universal_arm64_x64.dll
[12:23:39] Diff JIT Path: c:\work\runtime\artifacts\tests\coreclr\windows.x64.Checked\Tests\Core_Root\clrjit_universal_arm64_x64.dll
[12:23:39] Using MCH files:
[12:23:39]   c:\work\runtime\artifacts\spmi\mch\982ed1fd-7bf3-425b-8b8a-902873151e79.windows.arm64\benchmarks.run.windows.arm64.checked.mch
[12:23:39]   c:\work\runtime\artifacts\spmi\mch\982ed1fd-7bf3-425b-8b8a-902873151e79.windows.arm64\libraries.crossgen2.windows.arm64.checked.mch
[12:23:39] Running asm diffs of c:\work\runtime\artifacts\spmi\mch\982ed1fd-7bf3-425b-8b8a-902873151e79.windows.arm64\benchmarks.run.windows.arm64.checked.mch
[12:23:43] SuperPMI encountered missing data. Missing with base JIT: 47. Missing with diff JIT: 47. Total contexts: 34133.
[12:23:43] Running asm diffs of c:\work\runtime\artifacts\spmi\mch\982ed1fd-7bf3-425b-8b8a-902873151e79.windows.arm64\libraries.crossgen2.windows.arm64.checked.mch
[12:23:53] SuperPMI encountered missing data. Missing with base JIT: 55. Missing with diff JIT: 55. Total contexts: 238787.
[12:23:53] Asm diffs summary:
[12:23:53]   Summary Markdown file: c:\work\runtime\artifacts\spmi\diff_summary.2.md
[12:23:53]   No asm diffs

@TIHan
Copy link
Contributor Author

TIHan commented Oct 19, 2022

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

@azure-pipelines
Copy link

azure-pipelines bot commented Oct 19, 2022

Azure Pipelines successfully started running 2 pipeline(s).

BruceForstall
BruceForstall approved these changes Oct 19, 2022
View reviewed changes
@build-analysis build-analysis bot mentioned this pull request Oct 19, 2022
Closed
@TIHan @BruceForstall
Update src/coreclr/jit/emitarm64.cpp
a4e2c3c 
Co-authored-by: Bruce Forstall <brucefo@microsoft.com>
@TIHan TIHan merged commit 6c949f5 into dotnet:main Oct 19, 2022
@TIHan TIHan deleted the arm64-msl-16bit-fix branch October 19, 2022 19:27
@ghost ghost locked as resolved and limited conversation to collaborators Nov 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@BruceForstall BruceForstall BruceForstall approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@TIHan TIHan

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Test failure: JIT\\Regression\\JitBlue\\GitHub_23159\\GitHub_23159\\GitHub_23159.cmd

4 participants

@TIHan @AndyAyersMS @BruceForstall @markples