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.

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

[release/7.0] Use the register of CAST for contained index operator #75145

Merged
merged 1 commit into from
Sep 7, 2022
Merged

[release/7.0] Use the register of CAST for contained index operator #75145

merged 1 commit into from
Sep 7, 2022

Conversation

kunalspathak
Copy link
Member

@kunalspathak kunalspathak commented Sep 6, 2022 •
edited by carlossanlop
Loading

Backport of #74275

If index is contained and the underlying operator is GT_CAST we were not handling that case and that led to using REG_COUNT register from index in genScaledAdd() . I have fixed it and also added checks to see if more such cases are missing.

Fixes: #74117

@kunalspathak
Use the register of CAST for contained index operator (dotnet#74275)
9d5d565 
* Rename test files

* Fix index scale for cast

* Do not contain index if indir is not containable.

* Revert "Do not contain index if indir is not containable."

This reverts commit e79d17d92ceb0eed5ae1bfd03c2d1d6b171ab17f.

* Instead try to contain the LEA

* IsSafeToContainMem() check

* Do IsSafeToContainMem() only if needed

* Add test case

* Fix merge error

* fix the test case

* review comment
@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 Sep 6, 2022
@ghost ghost assigned kunalspathak Sep 6, 2022
@kunalspathak
Copy link
Member Author

@dotnet/jit-contrib

@ghost
Copy link

ghost commented Sep 6, 2022

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

Issue Details

If index is contained and the underlying operator is GT_CAST we were not handling that case and that led to using REG_COUNT register from index in genScaledAdd() . I have fixed it and also added checks to see if more such cases are missing.

Fixes: #74117

Author: kunalspathak
Assignees: -
Labels:

area-CodeGen-coreclr
Milestone: -

@carlossanlop carlossanlop changed the title Backport of "Use the register of CAST for contained index operator (#74275)" [release/7.0] Use the register of CAST for contained index operator Sep 6, 2022
@carlossanlop
Copy link
Member

@BruceForstall can we get a code review sign off?
@jeffschwMSFT can we get an approval?

jeffschwMSFT
jeffschwMSFT approved these changes Sep 6, 2022
View reviewed changes
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved. once we get a green ci we can merge.

@carlossanlop
Copy link
Member

There were a ton of llvmaot failures. Are they related to this change?

All failed with the same error code:

BEGIN : coreclr_initialize failed - error : 0x80004005 [/__w/1/s/src/mono/msbuild/aot-compile.proj]
END : coreclr_initialize failed - error : 0x80004005 [/__w/1/s/src/mono/msbuild/aot-compile.proj]
/__w/1/s/src/mono/msbuild/aot-compile.proj(19,9): error MSB3073: The command "/__w/1/s/artifacts/tests/coreclr/Linux.x64.Release/Tests/Core_Root/corerun /__w/1/s/artifacts/tests/coreclr/Linux.x64.Release/Tests/Core_Root/xunit.runner.utility.netcoreapp10.dll" exited with code 255.
    0 Warning(s)
    10770 Error(s)

Job results: https://dev.azure.com/dnceng-public/public/_build/results?buildId=5862&view=logs&j=ac444259-14dc-5e9e-4846-f4aabcc22cf0&t=616bd7d0-b458-5b0a-3812-dbea2ad61cca

@kunalspathak
Copy link
Member Author

Original PR didn't had those. The failure seems very fundamental that it failed to initialize coreclr. May be rerun?

@carlossanlop
Copy link
Member

@jkotas @MichalStrehovsky do the above llvmaot failures look familiar to you? Should I open an issue? I am unsure if any of the open issues with that error number is the same: https://github.com/dotnet/runtime/issues?q=is%3Aissue+is%3Aopen+0x80004005

@MichalStrehovsky
Copy link
Member

@jkotas @MichalStrehovsky do the above llvmaot failures look familiar to you? Should I open an issue?

llvmaot is Mono. Looks like #72114. Cc @lambdageek @SamMonoRT

@carlossanlop
Copy link
Member

Approved and signed off.
CI failure determined to be unrelated.
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit b357f02 into dotnet:release/7.0 Sep 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Oct 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jeffschwMSFT jeffschwMSFT jeffschwMSFT approved these changes

@BruceForstall BruceForstall BruceForstall approved these changes

Assignees

@kunalspathak kunalspathak

Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kunalspathak @carlossanlop @MichalStrehovsky @jeffschwMSFT @BruceForstall