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

JIT: Fix cross crossgen comparison failures #112078

Merged
merged 5 commits into from
Feb 3, 2025

Conversation

AndyAyersMS
Copy link
Member

@AndyAyersMS AndyAyersMS commented Feb 3, 2025

Fix a couple of issues that were causing cross-crossgen tests to fail

  • address mode formation was sensitive to the size of a constant handle
  • value histogram processing was always using 64 bit value sizes
  • transformation for down-counted loops was using host-sized -1.

Fixes the jit-related issues in #111972

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 3, 2025
Copy link
Contributor

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

@AndyAyersMS
Copy link
Member Author

/azp run runtime-coreclr crossgen2 outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Feb 3, 2025

cross crossgen results

Looks like xarch is now ok. OSX arm64 has a type layout verification failure. So maybe the jit side of things is in good shape.

20:20:39.940 Running test: JIT/opt/Devirtualization/Comparer_get_Default/Comparer_get_Default.dll

Assert failure(PID 13022 [0x000032de], Thread: 4596379 [0x46229b]): Verify_TypeLayout 'DoubleEnum' failed to verify type layout
    File: /Users/runner/work/1/s/src/coreclr/vm/jitinterface.cpp:13800
    Image: /private/tmp/helix/working/97380862/p/corerun

@AndyAyersMS AndyAyersMS changed the title Fix111972 Fix111972 -- Cross crossgen comparison failures Feb 3, 2025
@AndyAyersMS AndyAyersMS marked this pull request as ready for review February 3, 2025 15:29
@AndyAyersMS AndyAyersMS changed the title Fix111972 -- Cross crossgen comparison failures JIT: Fix cross crossgen comparison failures Feb 3, 2025
@AndyAyersMS
Copy link
Member Author

@dotnet/jit-contrib PTAL

@EgorBo
Copy link
Member

EgorBo commented Feb 3, 2025

Previous commit's diffs have regressions, is it because of the addressing modes or PGO-related changes?

@@ -1327,7 +1327,7 @@ bool ScalarEvolutionContext::Materialize(Scev* scev, bool createIR, GenTree** re
}
else
{
*result = m_comp->gtNewIconNode((ssize_t)cns->Value, scev->Type);
*result = m_comp->gtNewIconNode((target_ssize_t)cns->Value, scev->Type);
Copy link
Member

Choose a reason for hiding this comment

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

Can you make ScevConstant's constructor apply the sign extension for the genTypeSize(type) == 4 case instead? It better matches how GenTreeIntCon works (the constant is always stored sign extended).

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure I did what you expected, so take a look. My local setup got messed up so it will be a while before I can tell if this works.

Copy link
Member

Choose a reason for hiding this comment

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

That looks good to me

@AndyAyersMS
Copy link
Member Author

Previous commit's diffs have regressions, is it because of the addressing modes or PGO-related changes?

Ah, I hadn't looked at those. Would guess it is the PGO change, but let me verify.

@jkotas
Copy link
Member

jkotas commented Feb 3, 2025

@AndyAyersMS
Copy link
Member Author

Previous commit's diffs have regressions, is it because of the addressing modes or PGO-related changes?

Ah, I hadn't looked at those. Would guess it is the PGO change, but let me verify.

Spot checked some diffs, and yeah, looks like histogram computations were off before (always using host pointer sized bits to tabulate the histogram, then projecting to the low 32 bits). Typically value histograms on x64 were more "washed out" than they should have been).

It was likely hard to spot this during testing as it required histograms with mixtures of sizes and a good idea what the actual final %s should have looked like (eg the old interpretation is approximately correct, especially when the % of the dominant entry is high).

image

So the general sharpening in histogram % usually leads us to do some unrolling and so increase sizes (on x64 anyways).

Note the only x86 diffs are in the crossgen collection; when jitting it was reading the histograms properly.

@EgorBo
Copy link
Member

EgorBo commented Feb 3, 2025

ah, so some regressions are actually more value-probe driven intrinsic expanded? That explains some diffs I've looked into

@AndyAyersMS
Copy link
Member Author

/azp run runtime-coreclr crossgen2 outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS AndyAyersMS merged commit 6f7c6fb into dotnet:main Feb 3, 2025
155 of 158 checks passed
grendello added a commit to grendello/runtime that referenced this pull request Feb 4, 2025
* main: (30 commits)
  JIT: Optimize bit-wise AND with a constant mask in combination with a left shift in a compare (dotnet#111979)
  Change how we build the cross-OS DAC to support building in the VMR (dotnet#111927)
  Add Windows Server 2025 to test configurations (dotnet#111938)
  [PERF] Move performance testing YAML from dotnet/runtime to dotnet/performance (dotnet#111454)
  arm64: Add support for Bitwise OR NOT & XOR NOT (dotnet#111893)
  JIT: Fix cross crossgen comparison failures (dotnet#112078)
  Bump `StyleCop.Analyzers` to `1.2.0-beta.556` (dotnet#111278)
  Remove `RequiresProcessIsolation` on InterfaceFolding tests (dotnet#112098)
  Use hardlinks in helixpublishwitharcade (dotnet#112091)
  Update breaking change rules regarding byref/objref fields. (dotnet#112087)
  [daccess] Do not use USE_DAC_TABLE_RVA on Apple platforms (dotnet#112076)
  use collection syntax in illink (dotnet#108458)
  Include PDB for all TfmRuntimeSpecificPackageFile (dotnet#111879)
  [main] Update dependencies from dotnet/emsdk (dotnet#111690)
  Enable Mono tests (dotnet#111981)
  Let the debugger knows DATAS is on (dotnet#107115)
  Tests ran counter (dotnet#111145)
  Some System.Decimal performance improvements (dotnet#99212)
  [mono][mini] Remove support for the Xamarin.iOS and Xamarin.Mac assemblies in the AOT compiler. (dotnet#108886)
  Remove one usage of `Unsafe.AsPointer`. (dotnet#112079)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants