Stream: git-wasmtime

Topic: wasmtime / PR #4602 cranelift: Sign extend immediates in ...

Wasmtime GitHub notifications bot (Aug 03 2022 at 22:05):

afonso360 opened PR #4602 from iadd_imm_fix_sext to main:

:wave: Hey,

This PR alters the behaviour of *_imm instructions to sign extend their immediate argument when the control type is i128.

This comes from a fuzzer issue where the interpreter was sign extending the immediates but the legalizations were not.

Fixes: #4568

Wasmtime GitHub notifications bot (Aug 03 2022 at 22:07):

afonso360 updated PR #4602 from iadd_imm_fix_sext to main.

Wasmtime GitHub notifications bot (Aug 04 2022 at 07:16):

afonso360 updated PR #4602 from iadd_imm_fix_sext to main.

Wasmtime GitHub notifications bot (Aug 09 2022 at 19:15):

afonso360 edited PR #4602 from iadd_imm_fix_sext to main:

:wave: Hey,

This PR alters the behaviour of *_imm instructions to sign extend their immediate argument when the control type is i128.

This comes from a fuzzer issue where the interpreter was sign extending the immediates but the legalizations were not.

Fixes: #4568
Fixes: #4641

Wasmtime GitHub notifications bot (Aug 09 2022 at 19:26):

afonso360 updated PR #4602 from iadd_imm_fix_sext to main.

Wasmtime GitHub notifications bot (Aug 09 2022 at 19:27):

afonso360 has marked PR #4602 as ready for review.

Wasmtime GitHub notifications bot (Aug 09 2022 at 20:23):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Aug 09 2022 at 20:23):

jameysharp created PR review comment:

It looks like it's a behavioral change if we panic here. The outer match just skips the instruction if it doesn't have an expansion for it. Maybe that's why the previous implementation duplicated so much code? I think @cfallin probably needs to weigh in on how this case should be handled.

Gotta say, though, I _really_ like how much shorter your version is. I hope we can preserve that clarity even if this bit has to change.

It might help to introduce a function that's something like this (but I haven't tested this code, let alone compiled it):

fn imm_const(pos: &mut FuncCursor, arg: Value, imm: Imm64) -> Value {
    let ty = pos.func.dfg.value_type(arg);
    let imm = pos.ins().iconst(ty, imm);
    if ty == I128 {
        let imm = pos.ins().iconst(I64, imm);
        pos.ins().sextend(I128, imm)
    } else {
        pos.ins().iconst(ty.lane_type(), imm)
    }
}

Wasmtime GitHub notifications bot (Aug 09 2022 at 20:23):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Aug 09 2022 at 21:57):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 09 2022 at 21:57):

cfallin created PR review comment:

Yes, we don't want to hit an unimplemented!() here; I think we just want an empty match-arm body (_ => {}) instead. That way we still construct the imm but we just don't do anything with it.

Wasmtime GitHub notifications bot (Aug 09 2022 at 21:57):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 09 2022 at 22:10):

jameysharp created PR review comment:

Wouldn't an empty match-arm body there cause simple_legalize to loop, trying to legalize the same instruction repeatedly?

Wasmtime GitHub notifications bot (Aug 09 2022 at 22:10):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Aug 09 2022 at 22:13):

cfallin created PR review comment:

The toplevel loop is a while let Some(inst) = pos.next_inst(), so it's stepping through insts with no action taken in the loop body. Though actually the more precise thing to do is to replicate the fallback _ => { ... } below, which sets prev_pos first then continues, I think.

Wasmtime GitHub notifications bot (Aug 09 2022 at 22:13):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 09 2022 at 22:27):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Aug 09 2022 at 22:27):

jameysharp created PR review comment:

The statement after the match is pos.set_position(prev_pos); and the intent appears to be to re-examine the result of every legalization. So it looks to me like termination of this function relies on every match arm replacing the instruction that it originally matched on. But yes, copying the prev_pos assignment from the fallback case should work, I think.

Wasmtime GitHub notifications bot (Aug 10 2022 at 12:06):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Aug 10 2022 at 12:06):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Aug 10 2022 at 12:06):

akirilov-arm created PR review comment:

I don't have an opinion myself yet, but is sign-extending the best approach in this case?

Wasmtime GitHub notifications bot (Aug 10 2022 at 12:06):

akirilov-arm created PR review comment:

Ditto for all logical operations and rotations.

Wasmtime GitHub notifications bot (Aug 10 2022 at 12:06):

akirilov-arm created PR review comment:

Ditto.

Wasmtime GitHub notifications bot (Aug 15 2022 at 17:16):

afonso360 updated PR #4602 from iadd_imm_fix_sext to main.

Wasmtime GitHub notifications bot (Aug 15 2022 at 17:20):

afonso360 requested cfallin for a review on PR #4602.

Wasmtime GitHub notifications bot (Aug 15 2022 at 17:34):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 15 2022 at 17:34):

cfallin has enabled auto merge for PR #4602.

Wasmtime GitHub notifications bot (Aug 15 2022 at 18:08):

cfallin merged PR #4602.

Last updated: Dec 23 2024 at 12:05 UTC