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

x86: Add support for VMOVDQA instruction #279

Merged
merged 1 commit into from
Feb 8, 2023

Conversation

dsprenkels
Copy link
Contributor

On avx and avx2, the vmovdqa instruction performs aligned moves from/to memory.
Where vmovdqu will always copy data, no matter the alignment of the address. vmovdqa requires the address to be aligned corresponding to the size of the register where the data is being copied to/from. I.e., for an ymm register, the address needs to be aligned to 32 bytes. If the address is not aligned, this will trigger a general protection fault.

Review suggestions:

  • This patch adds support for an instruction that is meant to crash under certain circumstances. Should this be modeled by proofs? (I.e., should EC proofs also prove that the code will never crash, or do they assume that the code will never crash?)

Cc @vbgl

Copy link
Contributor

@bgregoir bgregoir left a comment

Choose a reason for hiding this comment

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

Thank for this contribution.
Actually, all memory access already check the alignment.
So it would be great to use this instruction in the generated code.
For the safety checker, yes it should be patched to ensure this. I hope this can be done in a generic way.

@dsprenkels
Copy link
Contributor Author

dsprenkels commented Nov 2, 2022

I tried but not could not find exactly where the alignment information is analyzed in the jasmin compiler. I can only find info on the alignment of the stack. Do you have any pointers?

@eponier
Copy link
Contributor

eponier commented Nov 8, 2022

In memory_model.v, writeV states that a write succeeds if the memory location is validw and validwP states that this implies that the memory address is aligned.

@dsprenkels
Copy link
Contributor Author

Would this imply that all memory accesses are always correctly aligned (because that is not true)? Or otherwise, how do we query that from the OCaml side?


My current train of thought:

jasminc will happily compile the following code:

export
fn test_vmovdqa(reg u64 p) {
        reg u256 r;

        r = (u256)[p + 16 * 0];
        (u256)[p + 16 * 1] = r;
}

which would result in the following assembly:

        .intel_syntax noprefix
        .text
        .p2align        5
        .globl  _test_vmovdqa
        .globl  test_vmovdqa
_test_vmovdqa:
test_vmovdqa:
        vmovdqu ymm0, ymmword ptr[rdi]
        vmovdqu ymmword ptr[rdi + 16], ymm0
        ret 

Which (after assembly) will crash with a general protection fault when it is run.

So, even though regular array indexing is aligned, not all memory-accesses are guaranteed to be aligned. In the linearization step, we have to figure out whether the assignment that is being linearized is aligned or not. However, I can not figure out how to determine if the alignment is known from an lval, wsize, or pexpr.

It's probably because I don't really have any experience with OCaml/Coq though ^^'

@vbgl
Copy link
Member

vbgl commented Nov 18, 2022

Asking the safety checker might give you some insight:

Analyzing function test_vmovdqa

*** Possible Safety Violation(s):
"huhu.jazz", line 6 (8-31): aligned pointer p + ((64u) (16 * 1)) u256

@vbgl
Copy link
Member

vbgl commented Feb 6, 2023

I’ve just try to always emit VMOVDQA instructions instead of VMOVDQU, to see in case it is faster.

I got a nice crash (SIGSEGV) there: https://github.com/formosa-crypto/libjade/blob/c5b9629c3fe6d92db8df12bf0e95d5edd76b0830/src/crypto_kem/kyber/kyber768/amd64/avx2/gen_matrix.jinc#L456, translated to: vmovdqa %xmm5,(%r11,%rdx,2)

where r11 = 0x7ffff78a9920 and rdx = 0xf.

Copy link
Member

@vbgl vbgl left a comment

Choose a reason for hiding this comment

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

I think that this PR should be merged: it is a nice addition that let the Jasmin programmers use VMOVDQA explicitly (in place of a plain assignment) when they believe this is good.

There is further work to do in order to allow the compiler to emit this instruction:

  • distinguish memory accesses introduced by the compiler that are proved to be aligned from the ones written by the programmer that target the external memory and are (currently) assumed to be aligned
  • relax the memory model to allow unaligned accesses and drop the assumption that accesses targeting the external memory are aligned

CI: https://gitlab.com/jasmin-lang/jasmin/-/pipelines/768753230

@vbgl vbgl merged commit e5fa667 into jasmin-lang:main Feb 8, 2023
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.

4 participants