Stream: git-wasmtime

Topic: wasmtime / PR #6938 riscv64: Use `PCRelLo12I` relocation ...

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2023 at 09:50):

afonso360 opened PR #6938 from afonso360:riscv-load-reloc to bytecodealliance:main:

:wave: Hey,

This is a small follow up to #6933 that restores our ability to use a relocation for loads that point to a label / constant pool label.

It looks like the relocation that I needed to use was already implemented so this ended up being a lot easier than I expected.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2023 at 09:50):

afonso360 requested elliottt for a review on PR #6938.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2023 at 09:50):

afonso360 requested wasmtime-compiler-reviewers for a review on PR #6938.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2023 at 13:41):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2023 at 13:41):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2023 at 13:41):

alexcrichton created PR review comment:

These match clauses here make me pause, if a label is available this means that it's ignoring offset and base, right? Could this perhaps assert that the offset is 0 and the base is not present? (or update the match clause's pattern match).

Also could the catch-all be updated to (None, None, None) and other patterns all asserted to not exist?

My guess is that this probably works as-is with what the backend already generates, so I'm thinking that this might be a good place for a few extra assertions that no other shapes are generated.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2023 at 16:37):

afonso360 updated PR #6938.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2023 at 16:42):

afonso360 updated PR #6938.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2023 at 16:44):

afonso360 has enabled auto merge for PR #6938.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2023 at 17:06):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2023 at 17:06):

jameysharp created PR review comment:

It wasn't until I stared at the implementations of get_base_register and get_label_with_sink that I realized exactly one of them will return Some for any address. This would be a little simpler maybe if there were one function that returned an enum with either a label or a register. I don't think that's necessary, but at least a comment might be nice; the updated pattern matching Some(_) made me scratch my head too.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2023 at 17:09):

afonso360 has disabled auto merge for PR #6938.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2023 at 17:16):

afonso360 updated PR #6938.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2023 at 17:18):

afonso360 created PR review comment:

Does this latest commit make things slightly more readable? I'm not a big fan of this match, but I also don't want to directly match the AMode since I think that would be worse.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2023 at 17:18):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2023 at 17:23):

alexcrichton created PR review comment:

Could this grow a None instead of _ to assert that label+base doesn't show up? Or, alternatively, could this assert that label is None here?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2023 at 17:23):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2023 at 17:23):

alexcrichton edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2023 at 17:26):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2023 at 17:26):

jameysharp created PR review comment:

I think the extra comments are very helpful, thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2023 at 17:40):

afonso360 updated PR #6938.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2023 at 17:49):

afonso360 has enabled auto merge for PR #6938.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2023 at 18:18):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2023 at 19:04):

afonso360 merged PR #6938.

Last updated: Oct 23 2024 at 20:03 UTC