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

Clarify that multiple R_RISCV_LO12_I/R_RISCV_LO12_S can share one single R_RISCV_HI20 #408

Merged
merged 2 commits into from
Dec 7, 2023

Conversation

MaskRay
Copy link
Collaborator

@MaskRay MaskRay commented Nov 22, 2023

This is most common on rv32 when loading a 8-byte aligned global variable, e.g.

double NaturallyAlignedScalar = 5.;
double accessNaturallyAlignedScalar() { return NaturallyAlignedScalar; }
//      lui     a1, %hi(NaturallyAlignedScalar)
//      lw      a0, %lo(NaturallyAlignedScalar)(a1)
//      lw      a1, %lo(NaturallyAlignedScalar+4)(a1)

double NaturallyAlignedArray[4] = { 3., 4., 5., 6. };
double accessNaturallyAlignedArray() {
  return NaturallyAlignedArray[0] + NaturallyAlignedArray[3];
}
//      lui     a2, %hi(NaturallyAlignedArray)
//      lw      a0, %lo(NaturallyAlignedArray)(a2)
//      lw      a1, %lo(NaturallyAlignedArray+4)(a2)
//      addi    a3, a2, %lo(NaturallyAlignedArray)
//      lw      a2, 24(a3)
//      lw      a3, 28(a3)

The HI20 values for the multiple fragments must be identical and all the relaxed global-pointer offsets must be in range.

…gle R_RISCV_HI20

This is most common on rv32 when loading a 8-byte aligned global
variable, e.g.
```
double NaturallyAlignedScalar = 5.;
double accessNaturallyAlignedScalar() { return NaturallyAlignedScalar; }
//      lui     a1, %hi(NaturallyAlignedScalar)
//      lw      a0, %lo(NaturallyAlignedScalar)(a1)
//      lw      a1, %lo(NaturallyAlignedScalar+4)(a1)

double NaturallyAlignedArray[4] = { 3., 4., 5., 6. };
double accessNaturallyAlignedArray() {
  return NaturallyAlignedArray[0] + NaturallyAlignedArray[3];
}
//      lui     a2, %hi(NaturallyAlignedArray)
//      lw      a0, %lo(NaturallyAlignedArray)(a2)
//      lw      a1, %lo(NaturallyAlignedArray+4)(a2)
//      addi    a3, a2, %lo(NaturallyAlignedArray)
//      lw      a2, 24(a3)
//      lw      a3, 28(a3)
```

The HI20 values for the multiple fragments must be identical and all the
relaxed global-pointer offsets must be in range.
@kito-cheng
Copy link
Collaborator

Generally LGTM, but I guess we may need to mention the alignment issue, the addends can't larger than alignment if the high part and low part using different addends, otherwise the high part may different.

e.g. symbol_address of foo is 0x17fc, which is an address align to 4 byte, not align to 8 byte, then it can't share to alignment between foo and foo+4, because (0x17fc + 0x800) >> 12 is 0x1 and (0x17fc + 4 + 0x800) >> 12 is 0x2.

@rui314
Copy link
Collaborator

rui314 commented Dec 7, 2023

The examples in this change raise the question of whether the example code is actually valid, and to address that question, we need to mention the alignment issue and its resolution, as pointed out by @kito-cheng. This discussion seems a bit derailed from the primary purpose of the psABI. Maybe just saying that there may be more than one R_RISCV_LO12_I/R_RISCV_LO12_S for a R_RISCV_HI20 is enough?

@MaskRay
Copy link
Collaborator Author

MaskRay commented Dec 7, 2023

The examples in this change raise the question of whether the example code is actually valid, and to address that question, we need to mention the alignment issue and its resolution, as pointed out by @kito-cheng. This discussion seems a bit derailed from the primary purpose of the psABI. Maybe just saying that there may be more than one R_RISCV_LO12_I/R_RISCV_LO12_S for a R_RISCV_HI20 is enough?

I have added "..., a condition met when the symbol is sufficiently aligned." Hopefully this is clear without derailing from the primary purpose.

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.

LGTM, and it's kind of clarification, which is already used in GCC and LLVM, so gonna merge, thanks :)

@kito-cheng kito-cheng merged commit 0091ace into riscv-non-isa:master Dec 7, 2023
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