Stream: wasmtime

Topic: push on master?

Alex Crichton (Apr 17 2020 at 19:25):

@Andrew Brown did you perhaps push to wasmtime master by accident?

Alex Crichton (Apr 17 2020 at 19:25):

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

Andrew Brown (Apr 17 2020 at 19:38):

Uh... Weird

Andrew Brown (Apr 17 2020 at 19:39):

Dan just approved a PR so I merged it but it was a tad old

Andrew Brown (Apr 17 2020 at 19:39):

So CI may not have run on it recently

Andrew Brown (Apr 17 2020 at 19:40):

I'll investigate in 15 min?

Alex Crichton (Apr 17 2020 at 19:43):

@Andrew Brown oh no worries

Alex Crichton (Apr 17 2020 at 19:43):

it may just be that it didn't show up as a merge commit

Alex Crichton (Apr 17 2020 at 19:43):

sounds good either way

Alex Crichton (Apr 17 2020 at 19:43):

and was probably just a normal merge conflict

Alex Crichton (Apr 17 2020 at 19:44):

oh d'oh if I click the commits they're obviously attached to a PR

Andrew Brown (Apr 17 2020 at 19:51):

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.

Chris Fallin (Apr 17 2020 at 19:52):

@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 { ... }

Chris Fallin (Apr 17 2020 at 19:53):

unless there's some other place const_addr is used that I don't know about yet :-)

Alex Crichton (Apr 17 2020 at 19:55):

@Andrew Brown oh I also sent https://github.com/bytecodealliance/wasmtime/pull/1546 just now

Alex Crichton (Apr 17 2020 at 19:55):

but if you have an actual implementation we should close that

Andrew Brown (Apr 17 2020 at 19:59):

Ha, no, I just pushed the same thing: https://github.com/bytecodealliance/wasmtime/pull/1548

Andrew Brown (Apr 17 2020 at 20:00):

I propose we merge one of those and then open an issue to implement this... it should be very similar to HeapAddr and TableAddr

Andrew Brown (Apr 17 2020 at 20:03):

What do you all think? @Alex Crichton, @Chris Fallin

Alex Crichton (Apr 17 2020 at 20:03):

:+1+ merged

Chris Fallin (Apr 17 2020 at 20:05):

@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?

Andrew Brown (Apr 17 2020 at 20:06):

Yeah :slight_smile:; currently the x86 backend is emitting constants in the code section as well, right after the function body

Andrew Brown (Apr 17 2020 at 20:06):

is that the same in the ARM backend?

Chris Fallin (Apr 17 2020 at 20:07):

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

Chris Fallin (Apr 17 2020 at 20:07):

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

Chris Fallin (Apr 17 2020 at 20:07):

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)

Andrew Brown (Apr 17 2020 at 20:08):

Ok, that is what ConstantPool is designed to do

Andrew Brown (Apr 17 2020 at 20:08):

...the "defer emission to the end" part

Chris Fallin (Apr 17 2020 at 20:09):

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

Andrew Brown (Apr 17 2020 at 20:09):

:+1:

Andrew Brown (Apr 17 2020 at 20:13):

In the meantime, can we implement ConstAddr?

Chris Fallin (Apr 17 2020 at 20:13):

https://github.com/bytecodealliance/wasmtime/issues/1549

Andrew Brown (Apr 17 2020 at 20:13):

Do you keep track of the pc-rel offset somewhere?

Chris Fallin (Apr 17 2020 at 20:14):

No, right now we hardcode "current inst + 8" because of the open-coded "LDR, jump around constant, constant" sequence

Andrew Brown (Apr 17 2020 at 20:15):

Ah, ok; so I guess unimplemented! is the right way to go for now.

Chris Fallin (Apr 17 2020 at 20:15):

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

Chris Fallin (Apr 17 2020 at 20:16):

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

Chris Fallin (Apr 17 2020 at 20:17):

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)

Andrew Brown (Apr 17 2020 at 20:17):

There's an idea

Andrew Brown (Apr 17 2020 at 20:17):

I mean, implementing ConstAddr shouldn't be the biggest priority but if that is easy to do...

Chris Fallin (Apr 17 2020 at 20:18):

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?

Andrew Brown (Apr 17 2020 at 20:19):

IIRC, other people mentioned other use cases but all I cared about was SIMD at the time :grinning_face_with_smiling_eyes:

Chris Fallin (Apr 17 2020 at 20:19):

Cool, well we should make it work on arm64 for sure then. Thanks!

Andrew Brown (Apr 17 2020 at 20:20):

:+1:

Last updated: Nov 22 2024 at 17:03 UTC