Skip to content
This repository has been archived by the owner on Apr 11, 2024. It is now read-only.

Add new relocation type support #50

Closed
wants to merge 1 commit into from

Conversation

lixing-star
Copy link

Add more relocation type for LoongArch

@xen0n
Copy link
Contributor

xen0n commented May 6, 2022

Overall in a good direction (and IMHO the direction everyone should have taken in the first place), thanks! A few suggestions though:

Some of the field names can be a bit hard to understand, consider L_LO12, L_HI20, H_LO20 and H_HI12; you'll have to mentally add the "lower/higher half" and "width" together to get a picture of the bit-field being expressed. It could be better to somehow directly show the 0(LO)/12/32/52 inside those names.

Plus, currently it's implied that a particular relocation type is somehow bound to a specific instruction, that's being used by the reference implementation (binutils) for the type today. This could change though, as future instructions emerge with similar or identical format.

I have drafted my scheme back in November 2021, which I posted here a while ago; the approach is extremely similar (IMO there's only one possible way to implement the more traditional scheme), but my version has adopted the operand slot naming scheme of loongarch-opcodes (which I'll be submitting to this repo shortly) and the more direct LO/12/32/52 names for bit-field offsets (width is implied by the destination slot). Hope this would be useful.

@xen0n
Copy link
Contributor

xen0n commented May 6, 2022

(In an additional note, the reason why I didn't particularly like the B16/B21/B26/BL26/JIRL names is they are too biased and specific. B26 and BL26 are in fact exactly identical, and B16 and JIRL are also identical. You really want to name the slots (“指令立即数域” as in the PR's content) instead of instruction names for them, but you don't have a naming scheme for these, so I guess that's why you have to invent names (and be different with others & prior work).

@ChenghuaXu
Copy link
Contributor

@yetist @SixWeining Please review this.

@xen0n
Copy link
Contributor

xen0n commented May 20, 2022

cc @xry111 @MaskRay: you may be interested in this too.

@MaskRay
Copy link

MaskRay commented Jul 19, 2022

(Context: I have major contributions to lld's aarch64,powerpc64,x86-32,x86-64 ports and am the main author of powerpc32/riscv ports.)

R_LARCH_PCREL_LO12U

Lowest 12-bit of 64-bit PC relative symbol, with check 12-bit unsigned overflow

Unsigned overflow checking is unusual. Re-think whether it is needed.
If needed, add notes which instructions use them.

Instead of the somewhat vague with check 20-bit signed overflow, just be specific: check xxx <= v < yyy.

Is there an R_LARCH_PCREL_LO12 with overflow check?
For LO12, I expect that there are two variants: one with overflow check, the other without.

Why do local-exec TLS relocation types have the TLSLE64 infix? 64 is not relevant.

R_LARCH_32_PCREL

The 64-bit counterpart is very useful in metadata.

R_LARCH_RELAX

If not used, don't add it now.

Note: A too-long relocation type name may have problems in the default (non-wide) output of readelf -r.

@xry111
Copy link
Contributor

xry111 commented Jul 19, 2022

(Context: I have major contributions to lld's aarch64,powerpc64,x86-32,x86-64 ports and am the main author of powerpc32/riscv ports.)

R_LARCH_PCREL_LO12U
Lowest 12-bit of 64-bit PC relative symbol, with check 12-bit unsigned overflow

Unsigned overflow checking is unusual. Re-think whether it is needed. If needed, add notes which instructions use them.

And, wait a minute: how could "lowest 12-bit" cause a "12-bit unsigned overflow"? This just does not make sense to me. Is this a typo or the intention is just "without overflow check"?

@MaskRay
Copy link

MaskRay commented Jul 20, 2022

(Context: I have major contributions to lld's aarch64,powerpc64,x86-32,x86-64 ports and am the main author of powerpc32/riscv ports.)

R_LARCH_PCREL_LO12U
Lowest 12-bit of 64-bit PC relative symbol, with check 12-bit unsigned overflow

Unsigned overflow checking is unusual. Re-think whether it is needed. If needed, add notes which instructions use them.

And, wait a minute: how could "lowest 12-bit" cause a "12-bit unsigned overflow"? This just does not make sense to me. Is this a typo or the intention is just "without overflow check"?

For example, aarch64 uses a pair of HI21/LO12_NC (NC for "no check") to encode a value which is representable in 33 bits.
If codegen emits just LO12, it indicates that the value must be represnetable with 12 bits.

@lzshhxx
Copy link
Contributor

lzshhxx commented Jul 20, 2022

R_LARCH_32_PCREL

The 64-bit counterpart is very useful in metadata.
Need a R_LARCH_64_PCREL?
I didn't realize how to use in metadata.

@xry111
Copy link
Contributor

xry111 commented Jul 20, 2022

Should we continue to review this or just close it as superseded by #57? This does not contain those "PCALA" relocs and it seems the binutils patches under review are using PCALA relocs.

@lzshhxx
Copy link
Contributor

lzshhxx commented Jul 20, 2022

Should we continue to review this or just close it as superseded by #57? This does not contain those "PCALA" relocs and it seems the binutils patches under review are using PCALA relocs.

I agree with you.

@ChenghuaXu
Copy link
Contributor

This pr should be closed @lixing-star , Please go to #57.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants