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

polkavm: Replace orc.b with an instruction sequence in the linker. #249

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aman4150
Copy link
Collaborator

@aman4150 aman4150 commented Jan 9, 2025

And get rid orc.b emulation code from the interpreter and the recompiler.

We are making this change because orc.b is rarely used and a very complicated instruction in itself, we don't need to support or upstream such a complicated instruction to Polkavm. Therefore let's emulate the instruction within the linker.

In addition, we are adding a warn print for the linker user to make them aware that orc.b is being replaced with an instruction sequence, and any perf benefit that the user is expecting would not be visible.

And get rid orc.b emulation code from the interpreter and the recompiler.

We are making this change because orc.b is rarely used and a very
complicated instruction in itself, we don't need to support or upstream
such a complicated instruction to Polkavm. Therefore let's emulate the
instruction within the linker.

In addition, we are adding a warn print for the linker user to make
them aware that orc.b is being replaced with an instruction sequence,
and any perf benefit that the user is expecting would not be visible.

Signed-off-by: Aman <[email protected]>
let mask_reg = if dst != src { src } else { Reg::E3 };
let range = if rv64 { 0..64 } else { 0..32 };

log::warn!("Emulating orc.b at {:?} with an instruction sequence", location);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should probably be a debug

// if OrCombineByte then emit function call
//

if kind == K::OrCombineByte {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can move the body of this if into the match below and avoid the unreachable!


if kind == K::OrCombineByte {
emit_or_combine_byte(current_location, dst, src, rv64, &mut emit);
emit(InstExt::nop());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why emit the extra nop?

@@ -2005,18 +2072,29 @@ where
};

use crate::riscv::RegKind as K;

//
// if OrCombineByte then emit function call
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment seems kinda superfluous (:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants