Page MenuHomePhabricator

[AMDGPU] Don't mark the .note section as ALLOC
ClosedPublic

Authored by Flakebi on Mar 17 2020, 5:19 AM.

Details

Summary

Marking a section as ALLOC tells the ELF loader to load the section into memory.
As we do not want to load the notes into VRAM, the flag should not be there.

On AMDHSA, .note is still marked as ALLOC, apparently this is currently
needed for OpenCL (see https://reviews.llvm.org/D74995).

Diff Detail

Event Timeline

Flakebi created this revision.Mar 17 2020, 5:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2020, 5:19 AM
Flakebi updated this revision to Diff 252067.Mar 23 2020, 9:08 AM

Fix formatting issue

Anyone willing to review this?

LGTM, although I don't understand the issue with OpenCL. Reading the comments on the linked review leave me more confused. If anyone from that discussion can weigh in I am happy to approve this.

t-tye added inline comments.Mar 26 2020, 9:23 PM
llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.cpp
441–443

Why does the AMDHSA ABI need this note section to be allocated?

@kzhuravl do you have any information about this?

nhaehnle accepted this revision.May 5 2020, 4:40 AM

LGTM. My suggestion would be to leave a TODO comment for the OpenCL thing and go ahead.

llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.cpp
441–443

Since we seem to have been unable to make progress on this OpenCL question, I would suggest to add a comment here saying that part of the OpenCL runtime relies on that, and add a link to that review (even though I agree that it's not particularly enlightening).

This revision is now accepted and ready to land.May 5 2020, 4:40 AM
This revision was automatically updated to reflect the committed changes.