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

Sign Extension in SBPFv2 #32924

Open
Tracked by #28755
Lichtso opened this issue Aug 21, 2023 · 6 comments
Open
Tracked by #28755

Sign Extension in SBPFv2 #32924

Lichtso opened this issue Aug 21, 2023 · 6 comments
Assignees

Comments

@Lichtso
Copy link
Contributor

Lichtso commented Aug 21, 2023

Problem

Currently in the RBPF VM some ALU32 instructions do sign extend their 32 bit results (outputs) to 64 bit.

These operations sign extend their results from 32 bit to 64 bit:
ADD32, SUB32, NEG32, MUL32, LMUL32, SDIV32, SREM32

These operations do not:
DIV32, MOD32, XOR32, OR32, AND32, LSH32, RSH32, ARSH32, MOV32, UDIV32, UREM32

It is likely that most programs do not ever use the upper 32 bit of a u32 or i32 and thus this sign extension work is wasted and could be omitted.

Proposed Solution

Add an explicit sign extension instruction and remove implicit sign extension of results, see:
https://github.com/solana-foundation/solana-improvement-documents/pull/87/files

@dmakarov
Copy link
Contributor

@nvjle, @jcivlin please, comment on this proposal.

@ksolana
Copy link
Contributor

ksolana commented Sep 2, 2023

is zero extend required by the specification? i see https://www.kernel.org/doc/html/latest/bpf/instruction-set.html#arithmetic-instructions and https://www.kernel.org/doc/Documentation/networking/filter.txt

  Still, the semantics of the original 32-bit ALU operations are preserved
  via 32-bit subregisters. All eBPF registers are 64-bit with 32-bit lower
  subregisters that zero-extend into 64-bit if they are being written to.
  That behavior maps directly to x86_64 and arm64 subregister definition, but
  makes other JITs more difficult.

@jcivlin
Copy link

jcivlin commented Sep 2, 2023

It is likely that most programs do not ever use the upper 32 bit of a u32 or i32 and thus this sign extension work is wasted and could be omitted.

We cannot make a decision on the ground of "likeliness". Either they are ever generated and then must be supported, or never generated and then must NOT be supported because we will not be able to test.
If it is a discussion about the cost of implementing over not implementing (and somehow imitating these sign extensions) then it will be instrumental to have some metics and evaluation, and they may be different, for example, performance, or required effort, or anything reliable. And if such metrics cannot be provided now, then I'd pick either hand - and let one who will be doing this to decide - and in parallel created an issue to reevaluate the decision once we have anything that provides more info.

@nvjle
Copy link

nvjle commented Sep 8, 2023

It is likely that most programs do not ever use the upper 32 bit of a u32 or i32 and thus this sign extension work is wasted and could be omitted.

We cannot make a decision on the ground of "likeliness". Either they are ever generated and then must be supported, or never generated and then must NOT be supported because we will not be able to test. If it is a discussion about the cost of implementing over not implementing (and somehow imitating these sign extensions) then it will be instrumental to have some metics and evaluation, and they may be different, for example, performance, or required effort, or anything reliable. And if such metrics cannot be provided now, then I'd pick either hand - and let one who will be doing this to decide - and in parallel created an issue to reevaluate the decision once we have anything that provides more info.

I am in agreement with Jan here. ISA extensions and changes should be driven by a quantitative approach and where clear need is demonstrated by data, and a cost-benefit analysis (e.g., as is standard in ISA/microarchitecture teams/implementations). Gut feelings and likelihood are not reliable indicators. Overall, given our current state (new runtime work needed, new ABI work needed, Move compiler implementation, and so forth), ISA changes are not a priority. The same goes for all of the recent "proposals" of ISA changes.

@dmakarov
Copy link
Contributor

dmakarov commented Sep 8, 2023

Add an explicit sign extension instruction and remove implicit sign extension of results.

This would change the semantics of ADD32, SUB32, NEG32, MUL32, LMUL32, SDIV32, SREM32 instructions. In addition the compiler would have to generate the explicit sing extension instruction every time it emits one of the ALU32 instructions to preserve the semantics of the operations. What would we gain from this change?

@Lichtso
Copy link
Contributor Author

Lichtso commented Sep 8, 2023

compiler would have to generate the explicit sing extension instruction every time it emits one of the ALU32 instructions to preserve the semantics of the operations

Not following every arithmetic instruction. Only when one casts from i32 to i64. Otherwise the upper 32 bit of i32 and u32 are undefined behavior from the source languages perspective.

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

No branches or pull requests

5 participants