wasmtime / PR #3062 Fix branching instructions in the int... · git-wasmtime

Hm, not totally sure about how to go about this; seems like there are a few options:

keep this as you implemented it, which is a bit more verbose

implement into_bool for integer types, but this may be a bit confusing because for other into_* functions we error when the type doesn't match

implement a ValueConversionKind::ToBoolean and just use arg(0)?.convert(ValueConversionKind::ToBoolean) here; I would probably lean this direction

Thoughts?

Wasmtime GitHub notifications bot (Jul 07 2021 at 16:27):

abrown created PR review comment:

@akirilov-arm, could you take a look at this? I think everything else makes sense but I don't know enough about the aarch64 backend to approve this part.

Wasmtime GitHub notifications bot (Jul 07 2021 at 16:27):

abrown created PR review comment:

For reference, the issue that fixes this is #2237.

Wasmtime GitHub notifications bot (Jul 07 2021 at 16:27):

abrown created PR review comment:

Ah, I see... I thought this meant "of" as in "a value of..." but I think it is an abbreviation of "overflows," right? Let's just go with the longer version to avoid confusion.

Wasmtime GitHub notifications bot (Jul 07 2021 at 16:27):

abrown created PR review comment:

BTW, I really appreciate you adding a bunch more CLIF tests!

Wasmtime GitHub notifications bot (Jul 07 2021 at 17:00):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 07 2021 at 17:00):

cfallin created PR review comment:

Could you add some more explanation of this sequence?

I think I understand what's going on here -- we have a 0/1 flag generated by cset that is the actual overflow indicator, then we cause the main compare instruction to subtract that value from INT32_MIN, so we get an overflow (or underflow actually) on the subtract-as-compare. But it's somewhat difficult to work out!

Wasmtime GitHub notifications bot (Jul 07 2021 at 18:37):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Jul 07 2021 at 18:37):

afonso360 created PR review comment:

implement into_bool for integer types, but this may be a bit confusing because for other into_* functions we error when the type doesn't match

I actually started by doing this, but it didn't feel "right" so I changed it.

Yeah, I think the ValueConversionKind is a good way to go, I'll update the PR

Wasmtime GitHub notifications bot (Jul 07 2021 at 18:59):

afonso360 created PR review comment:

Yes,

The "main" overflow check is the sub and cmp with the extends, however this produces the result as a ne flag.

However the main issue is that the caller of this function is expecting to use the vs flag, and we (currently) don't have a way to tell it to use another flag. So we set the overflow flag by subtracting either 1 / 0 from INT32_MIN.

I'll update the comments to reflect this.

Wasmtime GitHub notifications bot (Jul 07 2021 at 18:59):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Jul 07 2021 at 19:06):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 07 2021 at 19:06):

cfallin created PR review comment:

FWIW, that sounds like a reasonable refactor, if you'd prefer to go that way: return the (possibly modified) condition flag and use that in the caller. I think that at every place where we use lower_icmp, we have full control over what condition code we actually use, so this should be pretty reasonable, and would improve the generated code.

Wasmtime GitHub notifications bot (Jul 07 2021 at 19:26):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Jul 07 2021 at 19:26):

akirilov-arm created PR review comment:

... and would also simplify the explanation that @cfallin has asked for :-). My vote would go towards refactoring as well.

Wasmtime GitHub notifications bot (Jul 07 2021 at 19:41):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Jul 07 2021 at 19:41):

afonso360 created PR review comment:

Tested this again now, curiously, it works if we enable just the interpret test, but stops working with the run test.

test interpret
test run
target x86_64 machinst

function %brnz_b1(b1) -> b1 {
block0(v1: b1):
    brnz v1, block1
    jump block2

block1:
    v2 = bconst.b1 true
    return v2

block2:
    v3 = bconst.b1 false
    return v3
}
; run: %brnz_b1(true) == true
; run: %brnz_b1(false) == false

We get:

     Running `C:\Users\Afonso\CLionProjects\wasmtime\target\debug\clif-util.exe test ./test.clif`
thread 'worker #0' panicked at 'argument type mismatch: b8 != b1', cranelift\filetests\src\function_runner.rs:200:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
FAIL ./test.clif: panicked in worker #0: argument type mismatch: b8 != b1
1 tests
Error: 1 failure

Wasmtime GitHub notifications bot (Jul 07 2021 at 19:42):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Jul 07 2021 at 19:42):

afonso360 created PR review comment:

Ok, I'll update into that model instead.

Wasmtime GitHub notifications bot (Jul 08 2021 at 15:36):

afonso360 updated PR #3062 from interpreter-branches to main.

Wasmtime GitHub notifications bot (Jul 08 2021 at 15:58):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Jul 08 2021 at 15:58):

afonso360 created PR review comment:

I'm not 100% happy with how this turned out.

My issue is that this signature implies that we can change the output register, despite us never doing that. However changing IcmpResult into something like:
enum IcmpResult {
    Flag(IntCC),
    Register,
}
Also doesn't seem great, because now the output is inconsistent: if you request flag outputs you have to check the result and if you request register outputs you can ignore the result.

Do you guys have any suggestion on how to improve this?

This PR is also getting pretty big on the AArch64 side, if you would like me to separate it into 2 PR's let me know!

Wasmtime GitHub notifications bot (Jul 08 2021 at 20:03):

afonso360 updated PR #3062 from interpreter-branches to main.

Wasmtime GitHub notifications bot (Jul 09 2021 at 21:04):

afonso360 requested cfallin for a review on PR #3062.

Wasmtime GitHub notifications bot (Jul 09 2021 at 21:05):

afonso360 requested abrown for a review on PR #3062.

Wasmtime GitHub notifications bot (Jul 09 2021 at 22:44):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jul 09 2021 at 22:44):

abrown created PR review comment:

Well, I guess we'll just have to fix it all in #2237 :grinning_face_with_smiling_eyes:

Wasmtime GitHub notifications bot (Jul 09 2021 at 22:45):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jul 13 2021 at 09:55):

afonso360 updated PR #3062 from interpreter-branches to main.

Wasmtime GitHub notifications bot (Jul 13 2021 at 09:55):

afonso360 deleted PR review comment.

Wasmtime GitHub notifications bot (Jul 13 2021 at 23:10):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Jul 13 2021 at 23:10):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Jul 13 2021 at 23:10):

akirilov-arm created PR review comment:

At this point we should operate on Cond, not IntCC. Also, let's call this Condcode instead of Flag.

Wasmtime GitHub notifications bot (Jul 13 2021 at 23:10):

akirilov-arm created PR review comment:

Can we add an assertion that we have not asked for IcmpOutput::Flags in this case?

Wasmtime GitHub notifications bot (Jul 13 2021 at 23:19):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Jul 13 2021 at 23:19):

akirilov-arm created PR review comment:

Nit - the code structure looks like this:
  ...
} else if !cond {
  ...
} else {
  ...
Since you are changing the surrounding code significantly, could you also transform this, so that it does not use a negated condition:
  ...
} else if cond {
  ...
} else {
  ...

Wasmtime GitHub notifications bot (Jul 13 2021 at 23:22):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Jul 13 2021 at 23:22):

akirilov-arm created PR review comment:

I think that there should be just a single block like this one, right before the function returns.

Wasmtime GitHub notifications bot (Jul 13 2021 at 23:34):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Jul 13 2021 at 23:34):

akirilov-arm created PR review comment:

comparison

Also, there is no NE flag - it is called Z. However, a better way to phrase this is to say The result of this comparison is either the EQ or NE condition code ....

Wasmtime GitHub notifications bot (Jul 15 2021 at 21:20):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Jul 15 2021 at 21:20):

afonso360 created PR review comment:

We should emit the correct results if the caller asked for flags, we emit a subs.

Although, looking at this again, we can probably improve codegen by emitting a sub instead if the caller requested Registers.

I'll change that as well

Wasmtime GitHub notifications bot (Jul 15 2021 at 21:22):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Jul 15 2021 at 21:24):

afonso360 deleted PR review comment.

Wasmtime GitHub notifications bot (Jul 15 2021 at 23:25):

afonso360 updated PR #3062 from interpreter-branches to main.

Wasmtime GitHub notifications bot (Jul 15 2021 at 23:25):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Jul 15 2021 at 23:25):

afonso360 created PR review comment:

We don't do this everywhere, namely I128 gt/lt/etc and vector icmps, so I don't know how well this ends up being in practice. But I pushed a commit with this at the end, let me know which way you prefer it.

Wasmtime GitHub notifications bot (Jul 15 2021 at 23:28):

afonso360 updated PR #3062 from interpreter-branches to main.

Wasmtime GitHub notifications bot (Jul 15 2021 at 23:46):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Jul 15 2021 at 23:46):

akirilov-arm created PR review comment:

Are you saying that there are cases in which we pass IcmpOutput::Flags, but we return the result in a register (or vice versa)?

Wasmtime GitHub notifications bot (Jul 16 2021 at 00:39):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Jul 16 2021 at 00:39):

akirilov-arm created PR review comment:

This should be CondCode as well, for consistency with IcmpResult.

Wasmtime GitHub notifications bot (Jul 16 2021 at 00:39):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Jul 16 2021 at 00:39):

akirilov-arm created PR review comment:

Actually they are not - there is a very small number of optional instructions that perform partial NZCV updates, but they are quite specialized, so I do not anticipate that we are ever going to need them in Cranelift.

That's why I also suggested the term "condition code" instead of "flag".

Wasmtime GitHub notifications bot (Jul 16 2021 at 00:54):

akirilov-arm edited PR review comment.

Wasmtime GitHub notifications bot (Jul 16 2021 at 08:46):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Jul 16 2021 at 08:46):

afonso360 created PR review comment:

This is here to sort of warn callers that we may set flags that they do not expect, not that we really don't know what they are.

An example of this is the I128 EQ/NE implementation, we end that by emitting a CMN on the XOR of the inputs, this works for EQ/NE but if the caller tried to use any other cond code would result in wrong behaviour.

But I think we can clarify this by saying that callers can check the output cond and its negative.

Wasmtime GitHub notifications bot (Jul 16 2021 at 08:48):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Jul 16 2021 at 08:52):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Jul 16 2021 at 08:53):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Jul 16 2021 at 08:53):

afonso360 created PR review comment:

Thanks, that is much better!

Wasmtime GitHub notifications bot (Jul 16 2021 at 10:57):

afonso360 updated PR #3062 from interpreter-branches to main.

Wasmtime GitHub notifications bot (Jul 16 2021 at 10:58):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Jul 16 2021 at 11:00):

afonso360 requested akirilov-arm for a review on PR #3062.

Wasmtime GitHub notifications bot (Jul 16 2021 at 18:10):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Jul 16 2021 at 18:10):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Jul 16 2021 at 18:10):

akirilov-arm created PR review comment:

Typo - comparison.

Wasmtime GitHub notifications bot (Jul 16 2021 at 18:10):

akirilov-arm created PR review comment:

I just realized that this could be simplified further:
let mut should_materialize = output.reg().is_some();

Wasmtime GitHub notifications bot (Jul 16 2021 at 18:10):

akirilov-arm created PR review comment:

Typo - comparisons.

Wasmtime GitHub notifications bot (Jul 16 2021 at 18:14):

afonso360 updated PR #3062 from interpreter-branches to main.

Wasmtime GitHub notifications bot (Jul 16 2021 at 18:17):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Jul 16 2021 at 18:17):

akirilov-arm created PR review comment:

Oh, right, a bit of tunnel vision on my side - I was absolutely concentrating on the flags case.

Wasmtime GitHub notifications bot (Jul 16 2021 at 21:16):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Jul 19 2021 at 16:31):

abrown merged PR #3062.

Last updated: Nov 22 2024 at 16:03 UTC