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

The definition of the function is_aligned_addr overlaps with the function is_aligned #609

Open
KotorinMinami opened this issue Oct 31, 2024 · 4 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@KotorinMinami
Copy link
Contributor

In riscv_insts_base.sail:311-317 , we have:

function is_aligned(vaddr : xlenbits, width : word_width) -> bool =
  match width {
    BYTE   => true,
    HALF   => vaddr[0..0] == zeros(),
    WORD   => vaddr[1..0] == zeros(),
    DOUBLE => vaddr[2..0] == zeros(),
  }

but in riscv_mem.sail:42-43 , we have:

function is_aligned_addr forall 'n. (addr : xlenbits, width : int('n)) -> bool =
  unsigned(addr) % width == 0

Obviously the latter can implement the former, and there is code implementation redundancy between the two, of course we can implement is_aligned using is_aligned(vaddr, width) = is_aligned(vaddr, size_bytes(width)) to avoid somehow different definitions or duplicated definitions

@Timmmm
Copy link
Collaborator

Timmmm commented Oct 31, 2024

I agree. There are only 4 places where is_aligned() is called actually (3 for atomics, and 1 in check_misaligned()), so I would say we just remove it and replace it with is_aligned_addr(..., size_bytes(...)), and then probably rename is_aligned_addr to just is_aligned.

@arichardson arichardson added enhancement New feature or request good first issue Good for newcomers labels Oct 31, 2024
@Alasdair
Copy link
Collaborator

Alasdair commented Nov 1, 2024

One issue is the big integer modulus on the alignment check is very expensive, and it's effectively in a hot path where it gets called a lot. Back when I was optimising the Arm spec to improve the Linux boot times for our old POPL paper changing the alignment check was one of the biggest speedups.

@arichardson
Copy link
Collaborator

One issue is the big integer modulus on the alignment check is very expensive, and it's effectively in a hot path where it gets called a lot. Back when I was optimising the Arm spec to improve the Linux boot times for our old POPL paper changing the alignment check was one of the biggest speedups.

I imagine this could be fixed by rewriting the expression as unsigned(addr) & (width - 1) == 0?

@KotorinMinami
Copy link
Contributor Author

I imagine this could be fixed by rewriting the expression as unsigned(addr) & (width - 1) == 0?

I just don't know how the C code will handle it during execution, and function is_aligned_addr doesn't seem to limit the input range of width, which I think may need to be limited to $2^n, n=0,1,2,3...$, and really should be, by the definition of RISC-V address alignment, and by this optimization

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants