Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

riscv64: Add vconst lowerings #6324

Merged
merged 14 commits into from
May 9, 2023
Merged

riscv64: Add vconst lowerings #6324

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
867551a
riscv64: Use `LoadAddr` on `Load`/`Store`
afonso360 Apr 23, 2023
9b9268d
riscv64: Add I Type encoding
afonso360 Apr 23, 2023
e420731
riscv64: Add S Type encoding
afonso360 Apr 23, 2023
ac5fbd3
riscv64: Use `LoadAddr` on `VecLoad`/`VecStore`
afonso360 Apr 23, 2023
16687fa
riscv64: Add Const/Lable AModes
afonso360 Apr 24, 2023
0105eb8
riscv64: Add Label Address Generation
afonso360 Apr 24, 2023
df1bed7
riscv64: Add `vconst` support
afonso360 Apr 24, 2023
1048da3
riscv64: Use `unsigned_field_width` in encode
afonso360 May 5, 2023
5d1f8fe
riscv64: Use `WritableReg` in encode
afonso360 May 5, 2023
59b0983
riscv64: Deduplicate AMode formatting
afonso360 May 5, 2023
f3f2fd1
riscv64: Refcator VectorLoad/Store AMode Pattern matching
afonso360 May 5, 2023
c2a89d5
riscv64: Avoid passing `fp` and `sp` through the register allocator
afonso360 May 5, 2023
fabce61
riscv64: Fix `PCRel{Hi20,Lo12I}` relocation
afonso360 May 5, 2023
d8ae950
riscv64: Update PCRelLo12I Comment
afonso360 May 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions cranelift/codegen/src/isa/riscv64/inst/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1667,9 +1667,6 @@ pub enum LabelUse {

/// Equivalent to the `R_RISCV_PCREL_HI20` relocation, Allows setting
/// the immediate field of an `auipc` instruction.
///
/// Since we currently don't support offsets in labels, this relocation has
/// an implicit offset of 4.
PCRelHi20,

/// Equivalent to the `R_RISCV_PCREL_LO12_I` relocation, Allows setting
Expand Down Expand Up @@ -1827,15 +1824,22 @@ impl LabelUse {
}

LabelUse::PCRelHi20 => {
let offset = offset as u32 + 4;
let hi20 = offset & 0xFFFFF000;
let insn = (insn & 0xFFF) | hi20;
// See https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-elf.adoc#pc-relative-symbol-addresses
//
// We need to add 0x800 to ensure that we land at the next page as soon as it goes out of range for the
// Lo12 relocation. That relocation is signed and has a maximum range of -2048..2047. So when we get an
// offset of 2048, we need to land at the next page and subtract instead.
let offset = offset as u32;
let hi20 = offset.wrapping_add(0x800) >> 12;
let insn = (insn & 0xFFF) | (hi20 << 12);
buffer[0..4].clone_from_slice(&u32::to_le_bytes(insn));
}

LabelUse::PCRelLo12I => {
let offset = (offset as u32 + 4) & 0xFFF;
let insn = (insn & 0xFFFFF) | (offset << 20);
// We add 4 here since this relocation usually follows a PCRelHi20 relocation, at the previous
// instruction. So we need to account for the 4 byte difference in offsets there.
let lo12 = (offset + 4) as u32 & 0xFFF;
let insn = (insn & 0xFFFFF) | (lo12 << 20);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels backwards to me, but I think it's probably correct.

The instruction using Hi20 comes first, followed by the Lo12. So if the Hi20 label is at address x, then Lo12 is at address x+4.

Based on that, if we want to compute the same address for both labels, I expected that either Hi20 should use x+4, or Lo12 should use x-4.

Of the two instructions, the one which actually examines the program counter is auipc, so I think we need to compute the offset relative to that instruction. So I think you are correct that it's Lo12 that needs adjustment.

But I guess here we aren't given the address of the instruction, right? offset is the distance from this instruction to the address we want, or in other words, target_address - insn_address. So if insn_address is x-4, then offset is target_address - (x - 4), which is equivalent to (target_address - x) + 4.

And that's what you've implemented. If there's a way to make the comment more clear that'd be fantastic, but I'm at least reasonably convinced that this is correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the additional comment, @afonso360!

buffer[0..4].clone_from_slice(&u32::to_le_bytes(insn));
}
}
Expand Down