Skip to content

Conversation

namu-lee
Copy link
Contributor

There was an issue when emitting instructions for GT_JCMP nodes: 4-byte operands are always sign-extended before the comparison.

The commit suppresses emitting the 'sext.w' if the last instruction has already sign-extended the operands of JCMP. It cannot optimize out the sign-extension if a previously sign-extending instruction is not emitted right before the emission of the JCMP instruction, as it is implemented as a peephole optimization to conservatively preserve type safety.

The optimization slightly improves the performance of loop-intensive benchmark by reducing the instruction count of hot code, while not affecting the jit workload time.

@clamp03 @tomeksowi @SkyShield, @credo-quia-absurdum
part of #84834, cc @dotnet/samsung

* Peephole optimization for redundant sext.w
@Copilot Copilot AI review requested due to automatic review settings October 11, 2025 20:46
@github-actions github-actions bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 11, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Oct 11, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR optimizes RISC-V code generation by eliminating redundant sign-extension instructions (sext.w) in jump comparison (GT_JCMP) nodes. The optimization detects when a previous instruction has already sign-extended a register and avoids emitting another sign-extension instruction immediately after.

Key changes:

  • Added peephole optimization to detect redundant sign-extension instructions
  • Implemented helper functions to identify instructions that perform sign-extension
  • Applied the optimization in three specific code paths within jump comparison generation

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/coreclr/jit/emitriscv64.h Added function declarations for sign-extension detection and redundancy checking
src/coreclr/jit/emitriscv64.cpp Implemented emitInsIsSignExtend and isRedundantSignExtend helper functions
src/coreclr/jit/codegenriscv64.cpp Applied redundant sign-extension optimization in jump comparison code generation

Copy link
Contributor

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

@am11 am11 added the arch-riscv Related to the RISC-V architecture label Oct 11, 2025
namu-lee and others added 2 commits October 12, 2025 05:47
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
@clamp03
Copy link
Member

clamp03 commented Oct 13, 2025

Please check jit-format errors and fix them.

@tomeksowi
Copy link
Member

tomeksowi commented Oct 13, 2025

#118270 already removes some of the unnecessary sign-extensions. And not just for JCMP but for integer comparisons in general.

The problem with peep-hole'ing the instruction stream is that the argument may not be calculated in the last instruction. I also predict a problem with using full-register instruction variants for 32-bit values if possible (they have better compressed encodings, we may choose to do such optimizations in the future). It also won't recognize that integer primitive arguments are passed to functions sign-extended because the calling convention requires so.

I think the cost of added sign-extension nodes won't be high in the long run because the places where sign-extension is necessary are generally an exception, most 32-bit operations should already be sign-extended.

@namu-lee
Copy link
Contributor Author

#118270 already removes some of the unnecessary sign-extensions. And not just for JCMP but for integer comparisons in general.

The problem with peep-hole'ing the instruction stream is that the argument may not be calculated in the last instruction. I also predict a problem with using full-register instruction variants for 32-bit values if possible (they have better compressed encodings, we may choose to do such optimizations in the future). It also won't recognize that integer primitive arguments are passed to functions sign-extended because the calling convention requires so.

I think the cost of added sign-extension nodes won't be high in the long run because the places where sign-extension is necessary are generally an exception, most 32-bit operations should already be sign-extended.

Sorry I missed checking your PR. Closing this.

@namu-lee namu-lee closed this Oct 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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 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.

4 participants