Stream: git-wasmtime

Topic: wasmtime / PR #2071 machinst x64: Use xor/xorps/xorpd to ...

Wasmtime GitHub notifications bot (Jul 27 2020 at 17:30):

bnjbvr opened PR #2071 from x64-xor to main:

This shows how we can factor out helpers for knowing whether an instruction redefines its input instead of using it.

Wasmtime GitHub notifications bot (Jul 27 2020 at 17:30):

bnjbvr requested cfallin and julian-seward1 for a review on PR #2071.

Wasmtime GitHub notifications bot (Jul 27 2020 at 17:30):

bnjbvr requested cfallin and julian-seward1 for a review on PR #2071.

Wasmtime GitHub notifications bot (Jul 27 2020 at 17:58):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Jul 27 2020 at 17:58):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Jul 27 2020 at 17:58):

cfallin created PR Review Comment:

This reads a little awkwardly (three cheers for expression-ifs, but...). Perhaps add a helper to RegMemImm such as .to_reg() -> Option<Reg>, then you can do op == ... && src.to_reg() == Some(dst.to_reg()?

Wasmtime GitHub notifications bot (Jul 27 2020 at 17:58):

cfallin created PR Review Comment:

We should have a comment here describing what's going on, perhaps similar to my suggestion in @abrown's patch here.

Wasmtime GitHub notifications bot (Jul 27 2020 at 19:03):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Jul 27 2020 at 19:03):

abrown created PR Review Comment:

Another thought: why not make these methods of Inst? Then we can use this logic with inst.is_def() and avoid searching around for alu_rmi_r_is_def?

Wasmtime GitHub notifications bot (Jul 27 2020 at 19:06):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Jul 27 2020 at 19:06):

abrown created PR Review Comment:

What do you think of map_as_def? I don't feel too strongly about that but I think it would pretty helpful to explain here what is going on (e.g. we have to pretend the register is writable because map_def only accepts writable registers).

Wasmtime GitHub notifications bot (Jul 28 2020 at 08:20):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Jul 28 2020 at 08:20):

julian-seward1 created PR Review Comment:

I like the expression-if, but maybe it could be made clearer by changing the formal parameters thusly:

fn alu_rmi_r_is_def(rmi: &RegMemImm, reg: Writable<Reg>, op: AluRmiROpcode)

Other comments:

• I would find the name alu_rmi_r_is_const more natural; could that be considered?
• also op == AluRmiROpcode::Sub behaves similarly, and really we should handle that too, even though we don't expect anybody to actually use it

Wasmtime GitHub notifications bot (Jul 28 2020 at 08:24):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Jul 28 2020 at 08:24):

julian-seward1 created PR Review Comment:

Just as a sanity check .. do we need special-case handling for map_regs? IIUC, if there is no special-case handling then the mapping for the reg component of the insn will assert, complaining that the def and use maps are inconsistent .. yes? Can you confirm?

Wasmtime GitHub notifications bot (Jul 28 2020 at 08:33):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Jul 28 2020 at 08:33):

julian-seward1 created PR Review Comment:

Or maybe alu_rmi_r_produces_const ..

Wasmtime GitHub notifications bot (Jul 28 2020 at 08:41):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Jul 28 2020 at 08:41):

julian-seward1 created PR Review Comment:

Also cmpeqxx or whatever the recommended generate-all-1s idiom is. The Intel opt guide suggests this is recommended for Sandy Bridge onwards.

Wasmtime GitHub notifications bot (Jul 28 2020 at 08:43):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Jul 28 2020 at 13:58):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Jul 28 2020 at 13:58):

abrown created PR Review Comment:

I add that in #2060.

Wasmtime GitHub notifications bot (Jul 31 2020 at 13:50):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Jul 31 2020 at 13:50):

bnjbvr created PR Review Comment:

I like it!

Wasmtime GitHub notifications bot (Jul 31 2020 at 13:51):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Jul 31 2020 at 13:51):

bnjbvr created PR Review Comment:

I didn't run into any issues with this, and i could confirm it was probably used. This patch does adjust both reg usage collection and reg usage mapping, so nothing should be missing as far as i can tell.

Wasmtime GitHub notifications bot (Jul 31 2020 at 14:14):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Jul 31 2020 at 14:14):

bnjbvr created PR Review Comment:

Thanks for the comments! To sum up:

• created it as a method of Inst
• called produces_const()
• and added helper to_reg to both RegMem and RegMemImm

Wasmtime GitHub notifications bot (Jul 31 2020 at 14:27):

bnjbvr updated PR #2071 from x64-xor to main:

This shows how we can factor out helpers for knowing whether an instruction redefines its input instead of using it.

Wasmtime GitHub notifications bot (Jul 31 2020 at 20:17):

abrown merged PR #2071.

Last updated: Nov 22 2024 at 16:03 UTC