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

cranelift: Allow allocating heaps in clif filetests #7215

Open
afonso360 opened this issue Oct 11, 2023 · 2 comments
Open

cranelift: Allow allocating heaps in clif filetests #7215

afonso360 opened this issue Oct 11, 2023 · 2 comments
Labels
cranelift Issues related to the Cranelift code generator

Comments

@afonso360
Copy link
Contributor

afonso360 commented Oct 11, 2023

👋 Hey,

This was something that @jameysharp brought up during today's cranelift meeting.

Feature

We could add support to our runtests to allocate a heap memory region and pass it to the function under test.

Benefit

This allows us to do a few things:

  • We can start testing load+op fusion for vector instructions in the cranelift fuzzer for x64. We can't do this currently because they require the address to be aligned to 16bytes, and we can't guarantee that yet for stack addresses.

  • We can test our alias analysis memflags in the fuzzer using this. We could for example map different heaps to a different alias analysis bits, and ensure that our optimizations work.

  • We can also fuzz global values better. Since the heap is passed in via an argument, we can mark that argument as vmctx and start issuing global value loads based on that.

Implementation

We used to have something similar to this in #3302 that was then removed in #5386

We had an annotation like ; heap: static, size=0x1000, ptr=vmctx+0, bound=vmctx+8 which would pass the address of the heap via the vmctx parameter.

That used to involve some special handling for vmctx parameters that I think is incompatible with what we do today (don't quote me on this).

But we could do something similar like:

function %test(i64, i8) -> i64 {
...
}
; heap: <heap0> size=16, align=8
; run: %test(<heap0>, 10) == 10

And the <heap0> annotation would be resolved with the address of the heap when calling the test.

Alternatives

An alternative to testing load fusion for vectors, is to add stack alignment flags to our stack slots. But doing the alias analysis bits is slightly harder although there are probably workarounds there too.

@afonso360 afonso360 added the cranelift Issues related to the Cranelift code generator label Oct 11, 2023
@jameysharp
Copy link
Contributor

Your observations in #7225 reminded me that, in both fuzz tests and runtests, we can theoretically pass pointers from one function to another.

So one approach in runtests would be to get a stackslot pointer in one function and pass it to a callee, which can then treat the pointer as heap or vmctx or whatever. In fuzzgen, we could add a fake "address" type to the set of function parameter types we might generate; callers would fill such arguments in using the same selection logic as addresses for loads and stores, and callees would add those arguments to the set of addresses which loads and stores can choose from.

Since we don't do any inter-procedural optimization, that ensures that code generation can't do any hypothetical optimizations that would rely on knowing the pointer refers to the stack.

We still need the ability to specify alignment on stack slots though.

@afonso360
Copy link
Contributor Author

That's a great idea. We might not even have to do anything too hacky since we don't yet use reftypes for anything we could use those to signal, this is a valid address for loads and stores.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

No branches or pull requests

2 participants