Stream: git-wasmtime

Topic: wasmtime / Issue #2051 Aarch64 codegen quality: handle ad...

Wasmtime GitHub notifications bot (Jul 20 2020 at 21:41):

github-actions[bot] commented on Issue #2051:

Subscribe to Label Action

cc @bnjbvr

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64"

Thus the following users have been cc'd because of the following labels:

• bnjbvr: cranelift

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Jul 21 2020 at 19:11):

cfallin commented on Issue #2051:

@julian-seward1 I reworked the negation/casting a bit as per our discussion -- PTAL!

Wasmtime GitHub notifications bot (Jul 23 2020 at 05:40):

julian-seward1 commented on Issue #2051:

Thanks for the simplification. It is simpler, but tbh I am still unclear about the need for the masking. I had in mind the following; would it work?

assert!(twos_complement_bits <= 64);
if let Some(imm_value) = input_to_const(ctx, input) {
    let mut sign_extended =
        ((imm_value as i64) << (64 - twos_complement_bits)) >> (64 - twos_complement_bits);
    if let Some(i) = Imm12::maybe_from_u64(sign_extended as u64) {
        return (ResultRSEImm12::Imm12(i), false);
    }
    sign_extended = sign_extended.wrapping_neg();
    if let Some(i) = Imm12::maybe_from_u64(sign_extended as u64) {
        return (ResultRSEImm12::Imm12(i), true);
    }
}
(
    ResultRSEImm12::from_rse(put_input_in_rse(ctx, input, narrow_mode)),
    false,
)

Wasmtime GitHub notifications bot (Jul 23 2020 at 17:16):

cfallin commented on Issue #2051:

Ah, yes, I just realized that the masking doesn't actually matter: the Imm12's limit is lower than the maximum value for the masked-down width, so masking to the appropriate width first will not enable any additional constant values to be represented. (In the i8 case where that's not true, any negative value would also fit without negation.)

Wasmtime GitHub notifications bot (Jul 24 2020 at 18:31):

cfallin commented on Issue #2051:

Thanks! Updated naming to put_input_in_rse_imm12_maybe_negated; hopefully clearer.

Re: the sign extension: one way to think of it is that the sign extension does not affect the correctness of the add/sub, because narrower-than-64-bit values have undefined high bits. (So even adding 127 == -128 in i8-world is safe, because it produces the correct lowest byte.) The high bytes only matter where Imm12 is concerned -- it may falsely decide we can't use a 12-bit immediate because (actually ignored) high bits are set. So we need to sign extend before doing the 64-bit negation, so that we get a sign-extended positive value and can recognize small negative numbers as representable. (And why not mask the first, non-negated, case then? Because the constant with iconst should already be within range if positive.)

Last updated: Oct 23 2024 at 20:03 UTC