This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Add basic relocation support (currently used for dwarf only)
AbandonedPublic

Authored by kzhuravl on Apr 11 2016, 10:50 AM.

Details

Diff Detail

Event Timeline

kzhuravl updated this revision to Diff 53280.Apr 11 2016, 10:50 AM
kzhuravl retitled this revision from to [AMDGPU] Add basic relocation support (currently used for dwarf only).
kzhuravl updated this object.
kzhuravl added a reviewer: tstellarAMD.
kzhuravl added a subscriber: llvm-commits.

Last time I proposed a patch adding AMDGPU Relocation enums, Rafael asked for public documentation of these relocations. Do we have public docs yet?

arsenm added inline comments.Apr 25 2016, 5:43 PM
include/llvm/Object/RelocVisitor.h
417

The & 0xFFFFFFFF is redundant with the truncate to uint32_t

rafael added inline comments.Apr 29 2016, 8:25 AM
include/llvm/Support/ELFRelocs/AMDGPU.def
7

Document what these are.

lib/MC/MCObjectFileInfo.cpp
429–430 ↗(On Diff #53280)

Is anything testing this? Can in be in another patch?

lib/Target/AMDGPU/MCTargetDesc/AMDGPUELFObjectWriter.cpp
14

I don't think you have a test for the changes is thin file. Can they be in another patch.

include/llvm/Support/ELFRelocs/AMDGPU.def
7–10

Could you add the corresponding entries to the VariantKind enum in include/llvm/MC/MCExpr.h too, and also MCSymbolRefExpr::getVariantKindName(VariantKind Kind) in lib/MC/MCExpr.cpp

kzhuravl updated this revision to Diff 57840.May 19 2016, 12:53 PM
kzhuravl edited edge metadata.
kzhuravl marked 5 inline comments as done.

Update based on review feedback + added docs and missing test

lib/MC/MCExpr.cpp
280–282

I think for assembler support, you also need to add these to MCSymbolRefExpr::getVariantKindForName(StringRef Name)

kzhuravl updated this revision to Diff 58046.May 21 2016, 3:11 PM
kzhuravl marked an inline comment as done.

Address Tom's feedback

kzhuravl updated this revision to Diff 58239.May 24 2016, 8:32 AM

Make variant kind names consistent with relocation names

kzhuravl updated this revision to Diff 58801.May 27 2016, 9:56 AM

Rebase to tot

rafael edited edge metadata.May 27 2016, 10:06 AM
rafael added a subscriber: rafael.

If this is just for dwarf, why do you need high and lo relocations?

On x86_64 we use 32 and 64 relocations, but they apply to 32 and 64 bit
values.

In fact, you have no .s that produces a high relocation.

If this is just for dwarf, why do you need high and lo relocations?

On x86_64 we use 32 and 64 relocations, but they apply to 32 and 64 bit
values.

In fact, you have no .s that produces a high relocation.

This patch adds all the relocation definitions, but only uses the ones required for dwarf. I'm working on patches to add support for relocations of global variables, which use the high relocations.

Please leave the high one out of this patch.

It looks like the low one is actually just a 32 bit reloc. If not, add a
test case that shows the difference.

It sounds like we should add a new R_AMDGPU_32 reloc type to use for debug information in order to distinguish it from R_AMDGPU_32_LO/HI.

kzhuravl abandoned this revision.Jun 3 2016, 7:58 AM

Taken over by Tom

Taken over by Tom