Stream: git-wasmtime

Topic: wasmtime / PR #2251 refactor: retrieve immediates in new ...

Wasmtime GitHub notifications bot (Oct 01 2020 at 17:43):

abrown requested cfallin for a review on PR #2251.

Wasmtime GitHub notifications bot (Oct 01 2020 at 17:43):

abrown opened PR #2251 from access-immediates to main:

@cfallin, here is what I came up with after we discussed this last week. This change:

• moves DataValue into cranelift-codegen so that I can use it as a value wrapper
• gives InstructionData the ability to find immediate values in the format structures
• reworks LowerCtx::get_immediate to grab the value from the instruction (and the DataFlowGraph for vector immediates)

I felt like this approach provides the new backend a more abstract view of immediates instead of munging directly in the InstructionData but I didn't perform the refactor on the rest of the places the new backend grabs immediates because I wanted to get some feedback first.

Wasmtime GitHub notifications bot (Oct 01 2020 at 17:45):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 01 2020 at 17:45):

abrown created PR Review Comment:

Here and on line 295 I had to switch unsigned to signed--there are other ways to do this (bump up the bit width, e.g.) but I'm not really sure it matters (users of this probably know what to do?)

Wasmtime GitHub notifications bot (Oct 01 2020 at 18:44):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Oct 01 2020 at 18:44):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Oct 01 2020 at 18:44):

cfallin created PR Review Comment:

I'm not sure about unimplemented!() here -- usually that implies it would be possible but we just haven't done it -- perhaps None here, since we wrap imm_value() at a higher level? Or better, would it make sense to require a borrow of dfg.immediates as an argument to this function? Then we wouldn't need the two-level split logic between this match and LowerCtx's...

Wasmtime GitHub notifications bot (Oct 01 2020 at 18:44):

cfallin created PR Review Comment:

For clarity, maybe let imm: u32 = imm.into(); then we can write DataValue::from(imm as i32)?

Wasmtime GitHub notifications bot (Oct 01 2020 at 20:37):

abrown updated PR #2251 from access-immediates to main:

@cfallin, here is what I came up with after we discussed this last week. This change:

• moves DataValue into cranelift-codegen so that I can use it as a value wrapper
• gives InstructionData the ability to find immediate values in the format structures
• reworks LowerCtx::get_immediate to grab the value from the instruction (and the DataFlowGraph for vector immediates)

I felt like this approach provides the new backend a more abstract view of immediates instead of munging directly in the InstructionData but I didn't perform the refactor on the rest of the places the new backend grabs immediates because I wanted to get some feedback first.

Wasmtime GitHub notifications bot (Oct 01 2020 at 20:39):

abrown updated PR #2251 from access-immediates to main:

@cfallin, here is what I came up with after we discussed this last week. This change:

• moves DataValue into cranelift-codegen so that I can use it as a value wrapper
• gives InstructionData the ability to find immediate values in the format structures
• reworks LowerCtx::get_immediate to grab the value from the instruction (and the DataFlowGraph for vector immediates)

I felt like this approach provides the new backend a more abstract view of immediates instead of munging directly in the InstructionData but I didn't perform the refactor on the rest of the places the new backend grabs immediates because I wanted to get some feedback first.

Wasmtime GitHub notifications bot (Oct 01 2020 at 20:41):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 01 2020 at 20:41):

abrown created PR Review Comment:

Yeah, I was dubious about which way to go here. I felt like it was nice that this function didn't depend on the DataFlowGraph so I went with None.

Wasmtime GitHub notifications bot (Oct 01 2020 at 20:41):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 01 2020 at 20:41):

abrown created PR Review Comment:

...And documented it some more.

Wasmtime GitHub notifications bot (Oct 01 2020 at 20:42):

abrown updated PR #2251 from access-immediates to main:

@cfallin, here is what I came up with after we discussed this last week. This change:

• moves DataValue into cranelift-codegen so that I can use it as a value wrapper
• gives InstructionData the ability to find immediate values in the format structures
• reworks LowerCtx::get_immediate to grab the value from the instruction (and the DataFlowGraph for vector immediates)

I felt like this approach provides the new backend a more abstract view of immediates instead of munging directly in the InstructionData but I didn't perform the refactor on the rest of the places the new backend grabs immediates because I wanted to get some feedback first.

Wasmtime GitHub notifications bot (Oct 01 2020 at 21:01):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 01 2020 at 21:01):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 01 2020 at 21:01):

bjorn3 created PR Review Comment:

Please use Ieee32/Ieee64 instead of f32/f64. Otherwise NaN bits could get changed.

Wasmtime GitHub notifications bot (Oct 01 2020 at 21:24):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 01 2020 at 21:24):

abrown created PR Review Comment:

What do you think of https://doc.rust-lang.org/std/primitive.f32.html#method.from_bits? Looks to me like f32::from_bits (which is how we get from Ieee32 for this value) is not going to change NaN bits at all.

Wasmtime GitHub notifications bot (Oct 02 2020 at 05:45):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 02 2020 at 05:45):

bjorn3 created PR Review Comment:

The to_bits method can change it. x86 processors normalize it when moving out of an SSE register I think.

Wasmtime GitHub notifications bot (Oct 02 2020 at 16:23):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 02 2020 at 16:23):

abrown created PR Review Comment:

I don't know... Rust devs think the issue is "overblown": https://doc.rust-lang.org/src/core/num/f32.rs.html#700-702.

Wasmtime GitHub notifications bot (Oct 02 2020 at 16:42):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 02 2020 at 16:42):

bjorn3 created PR Review Comment:

That is a different issue. What I am talking about here is that for example when the user intended an sNaN it would quietly get turned into a regular NaN.

Wasmtime GitHub notifications bot (Oct 02 2020 at 16:45):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 02 2020 at 16:45):

abrown created PR Review Comment:

Zulip?

Wasmtime GitHub notifications bot (Oct 02 2020 at 16:50):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Oct 02 2020 at 16:50):

cfallin created PR Review Comment:

To add my 2¢ here: I don't think there's much cost to using Ieee32/Ieee64 (just some bitcasts), it has accessors to give us float values when we need them, it guarantees bit-for-bit accuracy otherwise as we manipulate code, and we already use it elsewhere for constants. I'd prefer the whole lowering path, from CLIF to the final point that we emit constants into the MachBuffer, to avoid potential machine-specific issues with f32/f64. Possiblyl @sunfishcode has thoughts (I know you've talked about NaNs and determinism before)?

Wasmtime GitHub notifications bot (Oct 02 2020 at 16:50):

cfallin edited PR Review Comment.

Wasmtime GitHub notifications bot (Oct 02 2020 at 17:14):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 02 2020 at 17:14):

abrown created PR Review Comment:

I'm not opposed to using Ieee32/Ieee64 here but I do want to sort out whether there are real issues that they avoid or not; if not, then I think Rust's f32/f64 are a bit simpler and perhaps we should be using those other places, e.g., for CLIF parsing.

Wasmtime GitHub notifications bot (Oct 02 2020 at 18:40):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 02 2020 at 18:40):

abrown created PR Review Comment:

Here is a summary of what @bjorn3 and I discussed on Zulip: because ARM machines might have default NaN mode set during compilation but not during runtime, the f32, if operated on (@bjorn3 thinks even possibly by moving to a register), could lose its signalling bit. This makes me lean toward switching to use Ieee32 but I would still like to hear @sunfishcode's opinion.

I'm still a bit mystified on why Rust core thinks this will not be an issue in their case: https://github.com/rust-lang/rust/commit/a2cdeb58f680b87b5bdb6a17cba857ac51307c8f#diff-14f327fd5e3b91eb2e9af0690596d156R284.

Wasmtime GitHub notifications bot (Oct 02 2020 at 18:46):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 02 2020 at 18:46):

bjorn3 created PR Review Comment:

I think what was thought to be a safety issue is sNaN causing a floating point trap as opposed to return sNaN or qNaN bits or even LLVM returning poison.

Wasmtime GitHub notifications bot (Oct 04 2020 at 19:27):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Oct 04 2020 at 19:27):

sunfishcode created PR Review Comment:

I favor Ieee32/Ieee64 here too.

It's my understanding that Rust as a language is still figuring out its strategy for NaN bits: https://github.com/rust-lang/unsafe-code-guidelines/issues/237 (to be sure, it's a hard problem). Until that's resolved, I'd prefer not to skate near the edge :-).

Wasmtime GitHub notifications bot (Oct 07 2020 at 17:00):

abrown updated PR #2251 from access-immediates to main:

@cfallin, here is what I came up with after we discussed this last week. This change:

• moves DataValue into cranelift-codegen so that I can use it as a value wrapper
• gives InstructionData the ability to find immediate values in the format structures
• reworks LowerCtx::get_immediate to grab the value from the instruction (and the DataFlowGraph for vector immediates)

I felt like this approach provides the new backend a more abstract view of immediates instead of munging directly in the InstructionData but I didn't perform the refactor on the rest of the places the new backend grabs immediates because I wanted to get some feedback first.

Wasmtime GitHub notifications bot (Oct 07 2020 at 17:09):

abrown created PR Review Comment:

This is the only change (and similarly on lines 237-238) that might be a bit risky in this commit: these functions are used to store and load values for the trampoline into a compiled function and I could repr(C) both Ieee32 and Ieee64 if there are doubts about the data layout.

Wasmtime GitHub notifications bot (Oct 07 2020 at 17:09):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 07 2020 at 17:33):

abrown updated PR #2251 from access-immediates to main:

@cfallin, here is what I came up with after we discussed this last week. This change:

• moves DataValue into cranelift-codegen so that I can use it as a value wrapper
• gives InstructionData the ability to find immediate values in the format structures
• reworks LowerCtx::get_immediate to grab the value from the instruction (and the DataFlowGraph for vector immediates)

I felt like this approach provides the new backend a more abstract view of immediates instead of munging directly in the InstructionData but I didn't perform the refactor on the rest of the places the new backend grabs immediates because I wanted to get some feedback first.

Wasmtime GitHub notifications bot (Oct 07 2020 at 17:34):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 07 2020 at 17:34):

abrown created PR Review Comment:

I added a repr(C) to Ieee32 and Ieee64 in the latest version of this commit; if they truly aren't necessary we can remove them later.

Wasmtime GitHub notifications bot (Oct 07 2020 at 19:17):

abrown merged PR #2251.

Last updated: Nov 22 2024 at 16:03 UTC