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

Specific AVX2 intrinsic inlining generates faulty x64 code #12835

Closed
damageboy opened this issue Jun 8, 2019 · 7 comments · Fixed by dotnet/coreclr#25135
Closed

Specific AVX2 intrinsic inlining generates faulty x64 code #12835

damageboy opened this issue Jun 8, 2019 · 7 comments · Fixed by dotnet/coreclr#25135
Assignees
Labels
area-CodeGen-coreclr bug
Milestone

Comments

@damageboy
Copy link
Contributor

@damageboy damageboy commented Jun 8, 2019

I have a short sequence of code that seems to generate faulty x64 code (and crashes the program).

I'm using the latest SDK bits as of this time as suggested by @tannergooding, dotnet --info says:

.NET Core SDK (reflecting any global.json):
 Version:   3.0.100-preview7-012287
 Commit:    f202b59402

Runtime Environment:
 OS Name:     ubuntu
 OS Version:  19.04
 OS Platform: Linux
 RID:         ubuntu.19.04-x64
 Base Path:   /home/dmg/dotnet/sdk/3.0.100-preview7-012287/

Host (useful for support):
  Version: 3.0.0-preview7-27806-02
  Commit:  90a101062a

.NET Core SDKs installed:
  2.1.603 [/home/dmg/dotnet/sdk]
  2.1.700 [/home/dmg/dotnet/sdk]
  3.0.100-preview5-011568 [/home/dmg/dotnet/sdk]
  3.0.100-preview7-012287 [/home/dmg/dotnet/sdk]

.NET Core runtimes installed:
  Microsoft.AspNetCore.All 2.1.11 [/home/dmg/dotnet/shared/Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.App 2.1.11 [/home/dmg/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.0.0-preview5-19227-01 [/home/dmg/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.0.0-preview7.19306.7 [/home/dmg/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 2.1.10 [/home/dmg/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.1.11 [/home/dmg/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.0.0-preview5-27626-15 [/home/dmg/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.0.0-preview7-27806-02 [/home/dmg/dotnet/shared/Microsoft.NETCore.App]

To install additional .NET Core runtimes or SDKs:
  https://aka.ms/dotnet-download

I have a complete-repro project setup for this issue.

The faulty code (as seen from lldb disassmbly is):

    0x7f91fc3d149a: shl    esi, 0x3
    0x7f91fc3d149d: movsxd rsi, esi
    0x7f91fc3d14a0: movabs rsi, 0x7f927405ece8
->  0x7f91fc3d14aa: vpmovzxbd ymm1, qword ptr [rsi + rsi] ; ymm1 = mem[0],zero,zero,zero,mem[1],zero,zero,zero,mem[2],zero,zero,zero,mem[3],zero,zero,zero,mem[4],zero,zero,zero,mem[5],zero,zero,zero,mem[6],zero,zero,zero,mem[7],zero,zero,zero

In this fragment, esi is initially the index into the permutation table I'm trying to load/convert, and is left shifted by 3 (as each entry is 8 bytes), but is then overwritten with the base address for the permutation table.

At this stage, the vpmovzxbd [rsi + rsi] is obviously totally bonkers and thankfully generated s segfault...

I think that the bug might be related to the base-address of the permutation array being a ReadOnlySpan<byte> embedded inside the executable, thanks to C# 7.3, as it seems the JIT is trying to correctly optimize it as a constant address during the compilation, but I could also be completely wrong about that last part.

Perviously @tannergooding opened up #25008, which is somewhat related to this issue as it related to how the offset calculation is expressed in the generated code.

@damageboy damageboy changed the title Specific AVX2 intrinsic inlining generated faulty x64 code Specific AVX2 intrinsic inlining generates faulty x64 code Jun 8, 2019
@mikedn
Copy link
Contributor

@mikedn mikedn commented Jun 8, 2019

Checked JIT asserts:

Assert failure(PID 12140 [0x00002f6c], Thread: 6644 [0x19f4]): Assertion failed '!"Expected empty defList at end of block"' in 'bugbugbug.Program:Main(ref)' (IL size 110)

    File: d:\projects\coreclr\src\jit\lsrabuild.cpp Line: 2210
    Image: D:\Projects\coreclr\bin\Product\Windows_NT.x64.Checked\CoreRun.exe

@tannergooding
Copy link
Member

@tannergooding tannergooding commented Jun 8, 2019

CC. @CarolEidt

@mikedn
Copy link
Contributor

@mikedn mikedn commented Jun 8, 2019

Same old problem - duplicate logic between lowering/LSRA/codegen. Lowering is deciding when something can be contained and then LSRA and codegen attempt to replicate its decision instead of just obeying what was already decided. And getting it wrong in the process.

In this case lowering has special logic for ConvertToVector256Int32 containment and LSRA doesn't have that and fails to call BuildAddrUses. And the codegen similarly fails to call genConsumeAddress.

I would expect that both LSRA and codegen notice that the operand is a contained address mode and do the right thing. What they instead is attempting to guess if the operand might be a contained address mode.

@AndyAyersMS
Copy link
Member

@AndyAyersMS AndyAyersMS commented Jun 12, 2019

cc @dotnet/jit-contrib. @CarolEidt is this something you plan to work on?

@mikedn
Copy link
Contributor

@mikedn mikedn commented Jun 12, 2019

@CarolEidt If you're busy with something else I think I can fix this later today. Let me know.

@CarolEidt
Copy link
Contributor

@CarolEidt CarolEidt commented Jun 12, 2019

@mikedn - thanks, but I can take a look at this (unless you're eager to do it - just let me know!)

@mikedn
Copy link
Contributor

@mikedn mikedn commented Jun 12, 2019

I think I'm going to take a look to see if it's possible to remove the duplicate logic between lowering/LSRA/codegen. However, I expect that to be a more intrusive fix and it's probably not the right time for that.

CarolEidt referenced this issue in CarolEidt/coreclr Jun 12, 2019
This adds an LEA case to both `LinearScan::BuildOperandUses` and `CodeGen::genConsumeRegs`.

Fix #25039
CarolEidt referenced this issue in CarolEidt/coreclr Jun 13, 2019
This adds an LEA case to both `LinearScan::BuildOperandUses` and `CodeGen::genConsumeRegs`.

Fix #25039
CarolEidt referenced this issue in dotnet/coreclr Jun 18, 2019
* Fix contained LEA handling

This adds an LEA case to both `LinearScan::BuildOperandUses` and `CodeGen::genConsumeRegs`.

Fix #25039
@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 31, 2020
@msftbot msftbot bot locked as resolved and limited conversation to collaborators Dec 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants