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

[arm64] JIT assertion failures for valid C# code #73804

Closed
adamsitnik opened this issue Aug 11, 2022 · 7 comments · Fixed by #73876
Closed

[arm64] JIT assertion failures for valid C# code #73804

adamsitnik opened this issue Aug 11, 2022 · 7 comments · Fixed by #73876
Assignees
Labels
arch-arm64 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@adamsitnik
Copy link
Member

In #73768 I've wrote quite complex, but valid (it defnitely works fine on x64) C# code that produces JIT assertion failures on arm64:

Sample run: https://github.com/dotnet/runtime/pull/73768/checks?check_run_id=7793120898

Sample log: https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-73768-merge-b3d703c3580445e6b2/JIT.1.Attempt.3/1/console.32c25b20.log?helixlogtype=result

    JIT\Performance\CodeQuality\BenchmarksGame\regex-redux\regex-redux-5\regex-redux-5.cmd [FAIL]
      
      Assert failure(PID 2736 [0x00000ab0], Thread: 11800 [0x2e18]): Assertion failed 'ins != INS_invalid' in 'System.SpanHelpers:LastIndexOfValueType(byref,short,int):int' during 'Generate code' (IL size 335; hash 0xbe43e860; FullOpts)
      
          File: D:\a\_work\1\s\src\coreclr\jit\hwintrinsiccodegenarm64.cpp Line: 282
          Image: D:\h\w\A7BB0946\p\corerun.exe
      
      
      Return code:      1
      Raw output file:      D:\h\w\A7BB0946\w\A93E0970\uploads\Reports\JIT.Performance\CodeQuality\BenchmarksGame\regex-redux\regex-redux-5\regex-redux-5.output.txt
      Raw output:

It would be great if someone from the @dotnet/jit-contrib Team could take a look and let me know whether I've hit a bug in JIT or my code is invalid. Thanks!

@adamsitnik adamsitnik added arch-arm64 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Aug 11, 2022
@adamsitnik adamsitnik added this to the 7.0.0 milestone Aug 11, 2022
@ghost
Copy link

ghost commented Aug 11, 2022

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

Issue Details

In #73768 I've wrote quite complex, but valid (it defnitely works fine on x64) C# code that produces JIT assertion failures on arm64:

Sample run: https://github.com/dotnet/runtime/pull/73768/checks?check_run_id=7793120898

Sample log: https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-73768-merge-b3d703c3580445e6b2/JIT.1.Attempt.3/1/console.32c25b20.log?helixlogtype=result

    JIT\Performance\CodeQuality\BenchmarksGame\regex-redux\regex-redux-5\regex-redux-5.cmd [FAIL]
      
      Assert failure(PID 2736 [0x00000ab0], Thread: 11800 [0x2e18]): Assertion failed 'ins != INS_invalid' in 'System.SpanHelpers:LastIndexOfValueType(byref,short,int):int' during 'Generate code' (IL size 335; hash 0xbe43e860; FullOpts)
      
          File: D:\a\_work\1\s\src\coreclr\jit\hwintrinsiccodegenarm64.cpp Line: 282
          Image: D:\h\w\A7BB0946\p\corerun.exe
      
      
      Return code:      1
      Raw output file:      D:\h\w\A7BB0946\w\A93E0970\uploads\Reports\JIT.Performance\CodeQuality\BenchmarksGame\regex-redux\regex-redux-5\regex-redux-5.output.txt
      Raw output:

It would be great if someone from the @dotnet/jit-contrib Team could take a look and let me know whether I've hit a bug in JIT or my code is invalid. Thanks!

Author: adamsitnik
Assignees: -
Labels:

arch-arm64, area-CodeGen-coreclr

Milestone: 7.0.0

@JulieLeeMSFT
Copy link
Member

@BruceForstall, PTAL.

@BruceForstall
Copy link
Member

You should never hit a JIT assert under any circumstance.

cc @tannergooding in case I'm wrong w.r.t. hw intrinsics

@SingleAccretion
Copy link
Contributor

SingleAccretion commented Aug 11, 2022

The issue here is that for some scalar intrinsics (here it is ArmBase_LeadinZeroCount) "the base type", which is also "the operation type" is derived from the the first operand. This operand can be small while the operation itself isn't.

Simple reproduction:

private static int Problem(short* s)
{
    return ArmBase.LeadingZeroCount(*s);
}

Edit: the presumable fix would be to use genActualType when deriving "base types" from operands.

@tannergooding
Copy link
Member

cc @tannergooding in case I'm wrong w.r.t. hw intrinsics

This is correct, assertions shouldn't be hit ever. For recursive intrinsics we should be at worst bailing out in import and causing it to throw a PNSE instead, for regular intrinsics we should at worst be falling back to the managed implementation.

@BruceForstall
Copy link
Member

What should be the IR behavior for short types in indirections? The incoming IL is:

ldarg.0
ldind.i2
call

So the ldind.i2 loads an int16 into int32 on the evaluation stack; an implicit cast to int.

If we don't recognize the intrinsic, we have:

  t3 =    LCL_VAR   long   V00 arg0         u:1 x0 (last use) REG x0 $80
       /--*  t3     long
  t4 = *  IND       short  REG x0 <l:$201, c:$200>
       /--*  t4     short
  t8 = *  PUTARG_REG int    REG x0
  t9 =    CNS_INT(h) long   0x7ff7fd7a7d20 ftn REG x1
       /--*  t9     long
 t10 = *  IND       long   REG x1
       /--*  t8     int    arg0 in x0
       +--*  t10    long   control expr
  t5 = *  CALL      int    System.Runtime.Intrinsics.Arm.ArmBase.LeadingZeroCount REG x0 $240

and generate a call like:

ldrsh   w0, [x0]
...
blr     x1

The PUTARG_REG is TYP_INT, but we don't have to generate any code for it.

When we recognize the intrinsic, we have:

  t3 =    LCL_VAR   long   V00 arg0         u:1 x0 (last use) REG x0 $80
       /--*  t3     long
  t4 = *  IND       short  REG x0 <l:$201, c:$200>
       /--*  t4     short
  t5 = *  HWINTRINSIC int     LeadingZeroCount REG x0 <l:$281, c:$280>

and generate the same:

ldrsh   w0, [x0]

before hitting the assert.

the presumable fix would be to use genActualType when deriving "base types" from operands.

Or do we need to insert a cast during importation?

@jakobbotsch
Copy link
Member

jakobbotsch commented Aug 12, 2022

What should be the IR behavior for short types in indirections?

They are zero/sign-extended to TYP_INT as part of the indirection.

Or do we need to insert a cast during importation?

That cast would just be removed by morph. Using genActualType should be the right fix (using non-actual types for a child node is almost never correct).

I've said this a few times, but I really wish GenTree::TypeGet() would always return actual types and that we would have specific type getters for the nodes that have support for small typed operations (e.g. GenTreeIndir::IndirType()). We time and time again hit these issues due to our representation.

BruceForstall added a commit to BruceForstall/runtime that referenced this issue Aug 12, 2022
The code already uses `emitActualTypeSize` in the scalar case;
this also uses `genActualType` to get the "actual" type of small
types when deciding the intrinsic base type, used in codegen.

Fixes dotnet#73804
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 12, 2022
BruceForstall added a commit that referenced this issue Aug 13, 2022
The code already uses `emitActualTypeSize` in the scalar case;
this also uses `genActualType` to get the "actual" type of small
types when deciding the intrinsic base type, used in codegen.

Fixes #73804
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 13, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-arm64 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants