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

Code clean up in AP for NonNull* #112027

Merged
merged 9 commits into from
Feb 11, 2025
Merged

Code clean up in AP for NonNull* #112027

merged 9 commits into from
Feb 11, 2025

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Jan 31, 2025

Expected to be zero-diff

@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 Jan 31, 2025
Copy link
Contributor

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

@EgorBo EgorBo marked this pull request as ready for review February 9, 2025 20:08
@EgorBo EgorBo closed this Feb 9, 2025
@EgorBo EgorBo reopened this Feb 9, 2025
@EgorBo
Copy link
Member Author

EgorBo commented Feb 9, 2025

No diffs as expected. This PR:

  1. Uses PeelOffset API where we manually try to accumulate an offset out of ADD(ADD(ADD(obj, cns1), cns2), cns3)
  2. Small code clean up in assertprop
  3. We didn't check "is offset too big?" where we should

@EgorBo
Copy link
Member Author

EgorBo commented Feb 10, 2025

PTAL @jakobbotsch @AndyAyersMS @dotnet/jit-contrib

ValueNum vnBase = vn;
target_ssize_t offset = 0;
vnStore->PeelOffsets(&vnBase, &offset);
if (fgIsBigOffset((size_t)offset))
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this check. fgBigOffset tells us if an indir off the final address will reliably tell us about nullness of the base, but here we're trying to find the base directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I think you're right, if Base is not-null then it should be fine, removed

@EgorBo
Copy link
Member Author

EgorBo commented Feb 11, 2025

/b-ag "osx arm64 jobs timeout"

@EgorBo
Copy link
Member Author

EgorBo commented Feb 11, 2025

/ba-g "osx arm64 jobs timeout"

@EgorBo EgorBo merged commit ed8df42 into dotnet:main Feb 11, 2025
108 of 112 checks passed
@EgorBo EgorBo deleted the ap-cleanuo branch February 11, 2025 08:13
grendello added a commit to grendello/runtime that referenced this pull request Feb 11, 2025
* main:
  Code clean up in AP for NonNull* (dotnet#112027)
  JIT: Invalidate LSRA's DFS tree if we aren't running new layout phase (dotnet#112364)
  Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250204.2 (dotnet#112339)
  Add doc on OS onboarding (dotnet#112026)
  Add `TypeName` APIs to simplify metadata lookup. (dotnet#111598)
  Internal monitor impl not using coop mutex causing deadlocks on Android. (dotnet#112358)
  Do not run NAOT arm64 OSX testing on all PRs (dotnet#112342)
  Special-case empty enumerables in AsyncEnumerable (dotnet#112321)
  Have mono handle ConvertToIntegerNative for Double and Single (dotnet#112206)
  Update dependencies from https://github.com/dotnet/arcade build 20250206.4 (dotnet#112338)
  System.Configuration.ConfigurationManager.Tests: use Assembly.Location to determine ThisApplicationPath. (dotnet#112231)
  Force write of local file header when "version needed to extract" changes (dotnet#112032)
  JIT: Don't reorder handler blocks (dotnet#112292)
  [RISC-V] Synthesize some floating constants inline (dotnet#111529)
  Enable `SA1000`: Spacing around keywords (dotnet#112302)
  Fix relocs for linux-riscv64 AOT (dotnet#112331)
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.

2 participants