Page MenuHomePhabricator

Partially revert D61491 "AMDGPU: Be explicit about whether the high-word in SI_PC_ADD_REL_OFFSET is 0"
ClosedPublic

Authored by foad on Aug 6 2019, 9:07 AM.

Details

Summary

D61491 caused us to use relocs when they're not strictly necessary, to
refer to symbols in the text section. This is a pessimization and it's a
problem for some loaders that don't support relocs yet.

Diff Detail

Repository
rL LLVM

Event Timeline

foad created this revision.Aug 6 2019, 9:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2019, 9:07 AM

Hmm, those test changes were unexpected.

Could you please change shouldEmitConstantsToTextSection to check for == AMDPAL instead? This really doesn't make sense for Mesa... and then add a test that explicit sets amdpal in the triple?

foad updated this revision to Diff 213833.Aug 7 2019, 3:21 AM

Only put constants in the text section on PAL.

foad added a comment.Aug 7 2019, 3:22 AM

Could you please change shouldEmitConstantsToTextSection to check for == AMDPAL instead? This really doesn't make sense for Mesa... and then add a test that explicit sets amdpal in the triple?

Done. global-constant.ll seems to test the HAS/non-HSA (now PAL/non-PAL) difference quite well. I don't know what to make of the other test changes -- I just did what I needed to to make them pass.

cwabbott accepted this revision.Sep 2 2019, 4:27 AM
cwabbott added a subscriber: cwabbott.

FWIW, this LGTM. Maybe it would be a good idea to add more lines to global-constant.ll to test radeonsi (amdgcn-- at the moment) and radv (amdgcn-mesa-mesa3d) under the NOPAL label.

I believe the no-initializer-constant-addrspace.ll test change is because it uses the external linkage and a non-PAL OS, so we emit an extra global offset table lookup once the workaround is removed, but that's normal and shouldn't matter for what it's testing. The R600 change is just due to it getting a separate section and also seems fine.

This revision is now accepted and ready to land.Sep 2 2019, 4:27 AM
This revision was automatically updated to reflect the committed changes.