@Andrew Brown did you perhaps push to wasmtime master by accident?
https://github.com/bytecodealliance/wasmtime/commits/master shows a few commits by you and CI is failing, and was wondering if this was an accident or intentional
Uh... Weird
Dan just approved a PR so I merged it but it was a tad old
So CI may not have run on it recently
I'll investigate in 15 min?
@Andrew Brown oh no worries
it may just be that it didn't show up as a merge commit
sounds good either way
and was probably just a normal merge conflict
oh d'oh if I click the commits they're obviously attached to a PR
So here's the resolution: since that PR was created we have added a brand new Aarch64 backend :smiley: which does not implement the new ConstAddr
instruction I added. I'll open a PR for that.
@Andrew Brown was just looking at that; since the new backend currently does its own thing for constants, you can probably just add a panic!()
to the big lowering match op { ... }
unless there's some other place const_addr
is used that I don't know about yet :-)
@Andrew Brown oh I also sent https://github.com/bytecodealliance/wasmtime/pull/1546 just now
but if you have an actual implementation we should close that
Ha, no, I just pushed the same thing: https://github.com/bytecodealliance/wasmtime/pull/1548
I propose we merge one of those and then open an issue to implement this... it should be very similar to HeapAddr
and TableAddr
What do you all think? @Alex Crichton, @Chris Fallin
:+1+ merged
@Andrew Brown just commented there; constant pool stuff will be a bit more complex on ARM because of limited address range for PC-rel addressing. Happy to talk more about this later though; looks like it's important for vector constants?
Yeah :slight_smile:; currently the x86 backend is emitting constants in the code section as well, right after the function body
is that the same in the ARM backend?
Oh, we do even better than that, and emit the constant right after the load-PC-rel-addr instruction and a branch around the constant
The "right" way to do it is to defer emission to the end, except for emitting islands of constants if the function is > 1MB large
But this works for now and let us avoid gnarly issues with that and with SpiderMonkey relocating the constant pool (which it does to insert its own epilogue)
Ok, that is what ConstantPool
is designed to do
...the "defer emission to the end" part
Got it; I have some thoughts on how to extend it for the "emit when we're about to get out of range" constant-island case; I'll create an issue to track this
:+1:
In the meantime, can we implement ConstAddr?
https://github.com/bytecodealliance/wasmtime/issues/1549
Do you keep track of the pc-rel offset somewhere?
No, right now we hardcode "current inst + 8" because of the open-coded "LDR, jump around constant, constant" sequence
Ah, ok; so I guess unimplemented! is the right way to go for now.
Yeah, I'll need to think about this a little bit, sorry! RIght now we basically have done our own thing to get stuff working for evaluation/MVP purposes. There's a bit more work to integrate fully here
Actually -- maybe it's not so bad if it's OK to duplicate the constant whenever referenced. Basically, a const_addr
would serve both to emit the constant and give an address to it
Then the emission is just the same as LoadConst64
except instead of LDR
(a normal 64-bit load from the embedded constant), we use ADR
(get address of PC-rel thing)
There's an idea
I mean, implementing ConstAddr shouldn't be the biggest priority but if that is easy to do...
Yup, I'll put this on my list. Question: is it used just for vector constants, or do we anticipate other uses (constant array data, ...) as well?
IIRC, other people mentioned other use cases but all I cared about was SIMD at the time :grinning_face_with_smiling_eyes:
Cool, well we should make it work on arm64 for sure then. Thanks!
:+1:
Last updated: Dec 23 2024 at 14:03 UTC