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.
afonso360 requested elliottt for a review on PR #6938.
afonso360 requested wasmtime-compiler-reviewers for a review on PR #6938.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
These match clauses here make me pause, if a
label
is available this means that it's ignoringoffset
andbase
, right? Could this perhaps assert that theoffset
is 0 and thebase
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.
afonso360 updated PR #6938.
afonso360 updated PR #6938.
afonso360 has enabled auto merge for PR #6938.
jameysharp submitted PR review.
jameysharp created PR review comment:
It wasn't until I stared at the implementations of
get_base_register
andget_label_with_sink
that I realized exactly one of them will returnSome
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 matchingSome(_)
made me scratch my head too.
afonso360 has disabled auto merge for PR #6938.
afonso360 updated PR #6938.
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.
afonso360 submitted PR review.
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 thatlabel
isNone
here?
alexcrichton submitted PR review.
alexcrichton edited PR review comment.
jameysharp submitted PR review.
jameysharp created PR review comment:
I think the extra comments are very helpful, thanks!
afonso360 updated PR #6938.
afonso360 has enabled auto merge for PR #6938.
alexcrichton submitted PR review.
afonso360 merged PR #6938.
Last updated: Dec 23 2024 at 12:05 UTC