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.

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

PIC medany: Fix parameters of load instruction #446

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

cmuellner
Copy link
Collaborator

@cmuellner cmuellner commented Jul 17, 2024

The explanation of the medium position independent code model
includes the instruction l[w|d] a0, a0, %pcrel_lo(.Ltmp3)
to calculate the address of a non-local symbol.
That's not the correct RISC-V syntax for a load instruction.
Let's fix that.

@cmuellner cmuellner requested a review from kito-cheng July 17, 2024 10:36
@@ -122,7 +122,7 @@ This model is similar to the medium any code model, but uses the

# Calculate address of non-local symbol
.Ltmp3: auipc a0, %got_pcrel_hi(symbol)
l[w|d] a0, a0, %pcrel_lo(.Ltmp3)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Uh, a0 is being use as the base address register here. It's just using the ISA manual's syntax rather than the canonical GNU one by putting the immediate after the register as a separate operand rather than parenthesising the register and putting the immediate before it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i.e.:

-         l[w|d] a0, a0, %pcrel_lo(.Ltmp3)
+         l[w|d] a0, %pcrel_lo(.Ltmp3)(a0)

is the right fix here

Copy link
Collaborator Author

@cmuellner cmuellner Jul 17, 2024

Choose a reason for hiding this comment

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

Right. Do you prefer to keep it that way or should I adjust to lw a0, %pcrel_lo(.Ltmp3)(a0)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since all examples in this section focus on RV32, this patch also replaces l[w|d] by lw.

They don't, they are just as valid on RV64 if symbol is an int.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(it's important here to be clear about the difference between the value and the address, rather than make everything a word, and we don't want to be assuming a specific XLEN either)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please keep the [w|d]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

I guessed that the sequence focuses on 32-bit loads/stores for simplicity (-> RV32).
But you are right that this only affects the size of the value.
When calculating the address, the addi (in the examples above) works for 32-bit and 64-bit addresses.

The explanation of the medium position independent code model
includes the instruction `l[w|d] a0, a0, %pcrel_lo(.Ltmp3)`
to calculate the address of a non-local symbol.
That's not the correct RISC-V syntax for a load instruction.
Let's fix that.

Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
Copy link
Collaborator

@kito-cheng kito-cheng left a comment

Choose a reason for hiding this comment

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

Thanks :)

@kito-cheng kito-cheng merged commit 9fec608 into riscv-non-isa:master Jul 18, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants