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

Fix missing zero-offset sequences and add checking #64805

Merged

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Feb 4, 2022

A couple changes:

  1. Add a checker which ensures we do not forget to call ExtendPtrVN (or equivalent) when necessary.
  2. Fix missing zero-offset sequences for ADDR(LCL_VAR) (no tests because reproducing bad codegen for this issue is "hard", and the checker asserts anyway).
  3. Fix missing NotAField zero-offset sequences. These are important to preserve because they carry the fact we could have had an overlapping field in the sequence. Tests added.
  4. Some printing enhancements for NotAField sequences, to make the dumps more informative.

Ref: #64773.

@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 4, 2022
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 4, 2022
@ghost
Copy link

ghost commented Feb 4, 2022

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

Issue Details

Namely, that we don't forget to call ExtendPtrVN when necessary. This would have, for example, caught the problem being fixed in #64501.

Ref: #64773.

No diffs are expected.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@SingleAccretion
Copy link
Contributor Author

There is a typo in this change, but the proper version found two cases where we miss field sequences. So we'll leave this in the draft state until fixes land.

@SingleAccretion SingleAccretion changed the title Add checking for zero-offset field sequence additions in VN Fix missing zero-offset sequences and add checking Feb 4, 2022
@SingleAccretion SingleAccretion force-pushed the Validate-ZeroOffs-FldSeq-Addition branch 3 times, most recently from e176e4b to 35ed939 Compare February 5, 2022 13:17
@SingleAccretion
Copy link
Contributor Author

SingleAccretion commented Feb 5, 2022

This is now blocked on #64501.

Edit: no more.

@SingleAccretion
Copy link
Contributor Author

Will assume OSX timeouts are not related (happening everywhere currently...).

Diffs are cases where we were numbering a real-world array of StructWithOverlappingFields (presumably, not in a way that lead to silent bad codegen).

@dotnet/jit-contrib

@SingleAccretion SingleAccretion marked this pull request as ready for review February 6, 2022 20:12
@jakobbotsch
Copy link
Member

/azp run Antigen, Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jakobbotsch
Copy link
Member

@SingleAccretion It looks like the fuzzlyn run hit an assert that was changed in #65387, can you investigate?

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@SingleAccretion
Copy link
Contributor Author

can you investigate?

Yes, of course.

@jakobbotsch
Copy link
Member

I believe Fuzzlyn/Antigen failures are preexisting.

@jakobbotsch jakobbotsch merged commit 1ea9cf4 into dotnet:main Feb 18, 2022
@SingleAccretion SingleAccretion deleted the Validate-ZeroOffs-FldSeq-Addition branch February 18, 2022 18:12
@ghost ghost locked as resolved and limited conversation to collaborators Mar 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants