Stream: git-wasmtime

Topic: wasmtime / issue #7113 riscv64: Improve icmp codegen

view this post on Zulip Wasmtime GitHub notifications bot (Sep 29 2023 at 16:50):

alexcrichton commented on issue #7113:

Ok looks like this won't work for at least one reason which is that u8 + u8 does not mask (similar for other non-32 and non-64 sizes).

@afonso360 do you know if this is a known issue with the risc-v backend? I'd be happy to help fix this up, but I'm not sure how deep this necessarily goes.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 29 2023 at 16:57):

elliottt commented on issue #7113:

Ok looks like this won't work for at least one reason which is that u8 + u8 does not mask (similar for other non-32 and non-64 sizes).

@afonso360 do you know if this is a known issue with the risc-v backend? I'd be happy to help fix this up, but I'm not sure how deep this necessarily goes.

The approach we were using in the past is to use normalize_cmp_value for the cases where we needed to make sure that a value had no non-zero upper bits. Probably that function should be renamed, and we should be more aggressive about using it when lowering operations that might rely on those upper bits.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 29 2023 at 19:44):

github-actions[bot] commented on issue #7113:

Subscribe to Label Action

cc @cfallin, @fitzgen

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

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

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

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Sep 29 2023 at 19:56):

alexcrichton commented on issue #7113:

Ok so I've looked around a bit, my conclusion is I don't know enough to be making changes here yet. I'm going to go back to the drawing board on some of this and see what I can figure out.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 29 2023 at 21:49):

afonso360 commented on issue #7113:

This is something that we really need! Currently we emit at least 4 instructions when we could do just one! Thanks for looking into this.

Also this just reminded me that I had a icmp PR that I had forgot about. Feel free to pick out anything you need from there!

One part that this commit removes is various sign-extensions around
icmp because, at least according to RISC-V's ABI, values are always
sign extended when sitting at rest in their registers. I'm not sure if
Cranelift respects this everywhere just yet,

@afonso360 do you know if this is a known issue with the risc-v backend? I'd be happy to help fix this up, but I'm not sure how deep this necessarily goes.

We actually don't follow this at all. We follow the general cranelift convention that upper bits are undefined and we clear / sign extend them in the ops that need it.

I looked up the ABI document and I could only find that sign extend requirement with regards to the calling convention, but as far as I know that is handled somewhere else in cranelift right?

view this post on Zulip Wasmtime GitHub notifications bot (Sep 29 2023 at 22:11):

alexcrichton commented on issue #7113:

Oh dear apologies for missing that! I'll probably restart from there.

I also need to investigate what's up with the ABI business and all that, but ok makes sense that Cranelift considers the upper bits undefined state. I know the ABI bits should do sign-extension as necessary, so I'll try to hunt that down and confirm and make sure it's all working.

Last updated: Oct 23 2024 at 20:03 UTC