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

Bitwise shifting instructions in EVM #215

Merged
merged 12 commits into from Sep 22, 2017

Conversation

Projects
None yet
6 participants
@axic
Copy link
Member

axic commented Feb 13, 2017

Introduce bitwise shifting.

(Replaces the old issue #145.)

@axic axic force-pushed the axic:bitwise-ops branch from d7919f0 to 17038a7 Feb 13, 2017

@axic axic referenced this pull request Feb 13, 2017

Closed

EVM opcodes: bitwise shifting #145

@axic axic changed the title Draft for bitwise shifting opcodes Bitwise shifting instructions in EVM Feb 13, 2017

@axic axic force-pushed the axic:bitwise-ops branch 2 times, most recently from 73cf336 to 4bab879 Feb 13, 2017

The `ROL` instruction (rotate left) pops 2 values from the stack, `arg1` and `arg2`, and pushes on the stack the second popped value `arg2` circular shifted to the left by the number of bits in the first popped value `arg1`.

```
(arg1 shl arg2) or (arg1 shr (256 - arg2)

This comment has been minimized.

@chriseth

chriseth Feb 24, 2017

Contributor

The most interesting use-cases for rolling are hash functions and other cryptographic primitives. In most cases, those require rolling inside 32 or 64 bits but seldomly inside 256 bits. Would it make sense to give another argument that determines the "width" of the roll? As the shift amount only takes a single byte, we could even encode both values inside the second argument, although that would make it less easy to use for variable roll amounts.

This comment has been minimized.

@gcolvin

gcolvin Feb 25, 2017

Collaborator

@chriseth A variable-width rotate instruction is interesting, though it might be better to wait for narrower data types.

@axic Are rotates really 4 times more expensive than a shift? Probably so. Most bigint libraries I've looked at, including Go, don't support them. So they will get implemented as you define them, e.g. (arg1 shl arg2) or (arg1 shr (256 - arg2). That also makes it tempting to leave them out, as the user can implement them that way for about the same performance and gas price. It's only for smaller data types where chips have native rotates that we really would need ROR and ROL.

This comment has been minimized.

@axic

axic Mar 23, 2017

Member

I tend to think that leaving it out is better since it is not supported by many underlying libraries, hence it will have to be implemented by hand in the VM. And when doing so we shouldn't give a subsidised gas cost - hence my suggested cost.

This comment has been minimized.

@axic

axic Apr 23, 2017

Member

As discussed offline, it makes no sense to include it in this form as 256-bit rotations are quite useless (= not used in any mainstream crypto algorithm).

Instead of having rotate which takes 3 parameters, the specific 32 or 64 bit rotates can be implemented with shifts in the code in question.

With the proposed SIMD operators rotate could be included.

@Arachnid

This comment has been minimized.

Copy link
Collaborator

Arachnid commented Apr 14, 2017

Please renumber this as eip-215 and we will merge as a draft.

@axic

This comment has been minimized.

Copy link
Member

axic commented Apr 22, 2017

I think it should be 145 based on the original issue id.

```

Notes:
- `arg2` is interpreted as unsigned number.

This comment has been minimized.

@chfast

chfast Apr 23, 2017

Contributor

This is the case for all shifts, so let's move it somewhere up into any generic description.

@chfast

chfast approved these changes Apr 23, 2017

Copy link
Contributor

chfast left a comment

Looks good.

@Arachnid

This comment has been minimized.

Copy link
Collaborator

Arachnid commented Apr 24, 2017

Are you happy for me to merge this?

@axic

This comment has been minimized.

Copy link
Member

axic commented Apr 26, 2017

@Arachnid seems like we have to clarify one part of SAR today.

@chfast

This comment has been minimized.

Copy link
Contributor

chfast commented Apr 26, 2017

Can we define SAR Python-style like floor(arg1 / 2**arg2). Otherwise it will difficult to specify it using existing opcodes.

@axic

This comment has been minimized.

Copy link
Member

axic commented Jul 3, 2017

Otherwise it will difficult to specify it using existing opcodes.

I think it wasn't specified in existing opcodes,udiv is not present. Though I agree we should be more specific in defining them.

@axic axic force-pushed the axic:bitwise-ops branch from 3db4f38 to 4218665 Jul 3, 2017

@axic

This comment has been minimized.

Copy link
Member

axic commented Jul 3, 2017

@chfast updated.

@Arachnid I think this should be ready now.

@gcolvin

This comment has been minimized.

Copy link
Collaborator

gcolvin commented Jul 3, 2017

Looks good to me. Still wish we had rotate, but it can wait.

@holiman

This comment has been minimized.

Copy link
Contributor

holiman commented Jul 28, 2017

LGTM!

@gcolvin

This comment has been minimized.

Copy link
Collaborator

gcolvin commented Jul 28, 2017

But where did the rotates go?

@axic

This comment has been minimized.

Copy link
Member

axic commented Jul 28, 2017

256-bit rotates are pretty much useless (at least today). Your SIMD proposal supports 32/64bit rotates, that is way more useful.

@gcolvin

This comment has been minimized.

Copy link
Collaborator

gcolvin commented Jul 28, 2017

Still would like them just for operational completeness, but I get what you are saying.

@axic

This comment has been minimized.

Copy link
Member

axic commented Jul 31, 2017

@chfast added the comparison

@axic

This comment has been minimized.

Copy link
Member

axic commented Aug 4, 2017

@chfast the operand order follows that of the other arithmetic opcodes, however I think we should reverse them, because I have a hunch it is a more natural usecase to shift a number already on the stack by a constant, than the other way around.

The current version results in a lot of DUP/SWAP cases, just like it is with DIV/SDIV.

@chfast

This comment has been minimized.

Copy link
Contributor

chfast commented Aug 4, 2017

I would go for it. This would also remove SWAP1 from my snippets.

@gcolvin

This comment has been minimized.

Copy link
Collaborator

gcolvin commented Aug 5, 2017

Yes, the operand order for DIV and SDIV are broken.

@chfast

This comment has been minimized.

Copy link
Contributor

chfast commented Aug 11, 2017

@axic As there are no more comments about this, can you revert the operand order?
I can also do this by creating a PR to this PR :D

@axic

This comment has been minimized.

Copy link
Member

axic commented Aug 11, 2017

@chfast I can do it, but if I won't by Tuesday can you push a PR? 😉

Also need to get tests done as a next step.

@axic

This comment has been minimized.

Copy link
Member

axic commented Sep 22, 2017

@chfast updated, please review

@@ -27,51 +27,51 @@ The following instructions are introduced:

### `0x1b`: `SHL` (shift left)

The `SHL` instruction (shift left) pops 2 values from the stack, `arg1` and `arg2`, and pushes on the stack the first popped value `arg1` shifted to the left by the number of bits in the second popped value `arg2`. The result is equal to
The `SHL` instruction (shift left) pops 2 values from the stack, `arg1` and `arg2`, and pushes on the stack the second popped value `arg2` shifted to the left by the number of bits in the first popped value `arg1`. The result is equal to

This comment has been minimized.

@chfast

chfast Sep 22, 2017

Contributor

I would skip "second popped" / "first popped". If the order is not clear, it should be clarified in the first part of the sentence.

How about

... pushes on the stack the result of the arg2 shifted to the left by the number of bits in arg1.

This comment has been minimized.

@axic

axic Sep 22, 2017

Member

Actually:

shifted to the left by arg1 number of bits

wouldn't be more readable?

@chfast

chfast approved these changes Sep 22, 2017

Copy link
Contributor

chfast left a comment

:shipit:

@axic

This comment has been minimized.

Copy link
Member

axic commented Sep 22, 2017

@Souptacular @Arachnid @gcolvin this should be ready to merge

@gcolvin
Copy link
Collaborator

gcolvin left a comment

Looks good, merge when you are ready, Alex.

@axic

This comment has been minimized.

Copy link
Member

axic commented Sep 22, 2017

@gcolvin it has to be someone with merging rights :)

@gcolvin gcolvin merged commit 35c7ade into ethereum:master Sep 22, 2017

@axic axic deleted the axic:bitwise-ops branch Sep 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment