Skip to content

Fix missing zero-offset sequences and add checking #64805

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.

Sign up for GitHub

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

Jump to bottom
Merged
jakobbotsch merged 5 commits into from
Feb 18, 2022
Merged

Fix missing zero-offset sequences and add checking #64805

jakobbotsch merged 5 commits into from
Feb 18, 2022

Conversation

@SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Feb 4, 2022 •
edited
Loading

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.

@ghost ghost added 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 labels 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 SingleAccretion mentioned this pull request Feb 4, 2022
Closed
SingleAccretion
SingleAccretion commented Feb 4, 2022
View reviewed changes
@SingleAccretion
Copy link
Contributor Author

SingleAccretion commented Feb 4, 2022

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 •
edited
Loading

This is now blocked on #64501.

Edit: no more.

@SingleAccretion SingleAccretion force-pushed the Validate-ZeroOffs-FldSeq-Addition branch from 35ed939 to 27d922b Compare February 5, 2022 18:11
@SingleAccretion SingleAccretion force-pushed the Validate-ZeroOffs-FldSeq-Addition branch from 27d922b to ae6c6e5 Compare February 6, 2022 15:42
@SingleAccretion
Copy link
Contributor Author

SingleAccretion commented Feb 6, 2022

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
@SingleAccretion SingleAccretion force-pushed the Validate-ZeroOffs-FldSeq-Addition branch from ae6c6e5 to 7ce21ab Compare February 12, 2022 10:24
@JulieLeeMSFT JulieLeeMSFT added this to the 7.0.0 milestone Feb 16, 2022
@jakobbotsch
Copy link
Member

jakobbotsch commented Feb 18, 2022

/azp run Antigen, Fuzzlyn

@azure-pipelines
Copy link

azure-pipelines bot commented Feb 18, 2022

Azure Pipelines successfully started running 2 pipeline(s).

@jakobbotsch
Copy link
Member

jakobbotsch commented Feb 18, 2022

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

jakobbotsch
jakobbotsch approved these changes Feb 18, 2022
View reviewed changes
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

SingleAccretion commented Feb 18, 2022

can you investigate?

Yes, of course.

@SingleAccretion SingleAccretion mentioned this pull request Feb 18, 2022
Merged
@jakobbotsch jakobbotsch reopened this Feb 18, 2022
@jakobbotsch
Copy link
Member

jakobbotsch commented Feb 18, 2022

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.

Reviewers

@jakobbotsch jakobbotsch jakobbotsch approved these changes

Assignees

@jakobbotsch jakobbotsch

@SingleAccretion SingleAccretion

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

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@SingleAccretion @jakobbotsch @JulieLeeMSFT