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

chore(interpreter): optimisation for BYTE, SHL, SHR and SAR #1418

Merged
merged 3 commits into from
May 24, 2024

Conversation

jpgonzalezra
Copy link
Contributor

@jpgonzalezra jpgonzalezra commented May 13, 2024

Hey guys, this is my first contribution here, so nice to meet you. I was working on this issue 1251

Motivation:
Investigate the performance of the BYTE, SHL, SHR and SAR OPCODEs and try to improvement

Solution:
The improvements I've tried to apply in the resulting assembly were:

  • reduced branching
  • optimized memory access
  • fewer instructions

I'm going to attach the before and after assembly for the opcode functions.

Byte

Before
After

SAR

Before
After

SHL

Before
After

SHR

Before
After

crates/interpreter/src/instructions/bitwise.rs Outdated Show resolved Hide resolved
};
let o1 = as_usize_saturated!(op1) % 32;
let byte_value = op2.byte(31 - o1);
*op2 = U256::from(byte_value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is right, but also not sure if there is a test for this in statetests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This way when 'o1' exceeds 31, the function returns ZERO but to ensure the implementation works as expected, can you suggest any specific tests we should do just in case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think so? 33 % 32 = 1 so it will be 31 - 1 byte instead of zero

Copy link
Contributor Author

@jpgonzalezra jpgonzalezra May 13, 2024

Choose a reason for hiding this comment

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

Yes, you're right. Sometimes we have things right in front of our eyes and they escape us. If you have any ideas on how to improve the method, please let me know, I am going to continue thinking about it ^^

Copy link
Contributor Author

@jpgonzalezra jpgonzalezra May 13, 2024

Choose a reason for hiding this comment

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

we can shifts the op2 right by calculated bits to align and isolate the desired byte using bitmask but I don't think it's better than byte(..)

something like this:

*op2 = if o1 < 32 {
      let shift = (31 - o1) * 8;
      U256::from((*op2 >> shift) & U256::from(0xFF))
  } else {
      U256::ZERO
  };

What do you think ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe byte is already optimal on since it will simply index into the value as a byte array. Thanks for the efforts anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'm going to leave this method as it was =)

Copy link
Collaborator

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

This looks fine to me, I have not verified all assembly but it looks like they would be improved.

crates/interpreter/src/instructions/bitwise.rs Outdated Show resolved Hide resolved
crates/interpreter/src/instructions/bitwise.rs Outdated Show resolved Hide resolved
}

/// EIP-145: Bitwise shifting instructions in EVM
pub fn shr<H: Host + ?Sized, SPEC: Spec>(interpreter: &mut Interpreter, _host: &mut H) {
check!(interpreter, CONSTANTINOPLE);
gas!(interpreter, gas::VERYLOW);
pop_top!(interpreter, op1, op2);
*op2 >>= as_usize_saturated!(op1);
let shift = as_usize_saturated!(op1);
*op2 = if shift < 256 {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@jpgonzalezra jpgonzalezra May 16, 2024

Choose a reason for hiding this comment

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

Nice. so, are we on the right track?

Copy link
Member

@rakita rakita left a comment

Choose a reason for hiding this comment

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

lgtm

@rakita rakita merged commit ff2dcf5 into bluealloy:main May 24, 2024
25 checks passed
This was referenced Jun 8, 2024
This was referenced Jun 17, 2024
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.

3 participants