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

[RISC-V] Remove unnecessary assertion from emitOutputInstr #98484

Merged
merged 3 commits into from
Feb 19, 2024

Conversation

tomeksowi
Copy link
Contributor

@tomeksowi tomeksowi commented Feb 15, 2024

Remove unnecessary assertion introduced in #98226. It fired a false positive when W^X was enabled.

Part of #84834
cc @wscho77 @HJLeee @clamp03 @JongHeonChoi @t-mustafin @gbalykov @viewizard @ashaurtaev @brucehoult @sirntar @yurai007 @Bajtazar @bartlomiejko

It fired a false positive when W^X was enabled
@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 15, 2024
@ghost
Copy link

ghost commented Feb 15, 2024

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

Issue Details

Remove unnecessary assertion. It fired a false positive when W^X was enabled.

Also, reuse emitOutputLong instead of creating our own for RISC-V.

Part of #84834
cc @wscho77 @HJLeee @clamp03 @JongHeonChoi @t-mustafin @gbalykov @viewizard @ashaurtaev @brucehoult @sirntar @yurai007 @Bajtazar

Author: tomeksowi
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@clamp03 clamp03 added the arch-riscv Related to the RISC-V architecture label Feb 15, 2024
@ryujit-bot
Copy link

Diff results for #98484

Throughput diffs

Throughput diffs for linux/arm64 ran on windows/x64

MinOpts (-0.01% to +0.00%)
Collection PDIFF
libraries.pmi.linux.arm64.checked.mch -0.01%

Throughput diffs for osx/arm64 ran on windows/x64

MinOpts (-0.00% to +0.01%)
Collection PDIFF
realworld.run.osx.arm64.checked.mch +0.01%

Throughput diffs for windows/arm64 ran on windows/x64

MinOpts (-0.01% to +0.00%)
Collection PDIFF
realworld.run.windows.arm64.checked.mch -0.01%

Details here


There shouldn't be a problem when we introduce compressed instructions, VF2 supports unaligned stores (sw)
Also, replace compile-time conditions with static_asserts.
@ryujit-bot
Copy link

Diff results for #98484

Throughput diffs

Throughput diffs for osx/arm64 ran on windows/x64

MinOpts (-0.00% to +0.01%)
Collection PDIFF
realworld.run.osx.arm64.checked.mch +0.01%

Throughput diffs for windows/arm64 ran on windows/x64

MinOpts (-0.01% to +0.00%)
Collection PDIFF
realworld.run.windows.arm64.checked.mch -0.01%

Details here


@ryujit-bot
Copy link

Diff results for #98484

Throughput diffs

Throughput diffs for osx/arm64 ran on windows/x64

MinOpts (-0.00% to +0.01%)
Collection PDIFF
realworld.run.osx.arm64.checked.mch +0.01%

Throughput diffs for windows/arm64 ran on windows/x64

MinOpts (-0.00% to +0.01%)
Collection PDIFF
libraries.pmi.windows.arm64.checked.mch +0.01%

Details here


@tomeksowi tomeksowi marked this pull request as ready for review February 15, 2024 21:36
Copy link

@bartlomiejko bartlomiejko left a comment

Choose a reason for hiding this comment

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

LGTM

@ryujit-bot
Copy link

Diff results for #98484

Throughput diffs

Throughput diffs for linux/arm64 ran on windows/x64

MinOpts (-0.01% to +0.00%)
Collection PDIFF
libraries.pmi.linux.arm64.checked.mch -0.01%
realworld.run.linux.arm64.checked.mch -0.01%

Details here


@brucehoult
Copy link

LGTM

@github-actions github-actions bot locked and limited conversation to collaborators Apr 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-riscv Related to the RISC-V architecture 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.

8 participants