This is an archive of the discontinued LLVM Phabricator instance.

[LLD] [COFF] Create range extension thunks for ARM64
ClosedPublic

Authored by mstorsjo on Feb 1 2019, 2:29 AM.

Details

Summary

On ARM64, this is normally necessary only after a module exceeds 128 MB in size (while the limit for thumb is 16 MB). For conditional branches, the range limit is only 1 MB though (the same as for thumb), and for the tbz instruction, the range is only 32 KB, which allows for a test smaller than the full 128 MB.

This fixes PR40467.

Diff Detail

Event Timeline

mstorsjo created this revision.Feb 1 2019, 2:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2019, 2:29 AM
mstorsjo updated this revision to Diff 184706.Feb 1 2019, 2:36 AM
mstorsjo edited the summary of this revision. (Show Details)

Fixed a typo in the range checks for adding thunks for conditional branches. For the tbz instruction with the relocation IMAGE_REL_ARM64_BRANCH14, the range actually only is +/- 32 KB, making the testcase produce slightly smaller intermediates.

ruiu added inline comments.Feb 1 2019, 9:38 AM
COFF/Writer.cpp
374–385

This is the only place you are using RangeExtensionThunk{ARM,ARM64} as RangeExtensionThunk, but you can easily avoid it. So, RangeExtensionThunk class doesn't seem necessary. Can you remove that class and make RangeExtensionThunk{ARM,ARM64} directly inherit Chunk, so that you have shallower class hierarchy?

mstorsjo updated this revision to Diff 184787.Feb 1 2019, 10:37 AM
mstorsjo marked an inline comment as done.

Removed unnecessary intermediate class, removed a test for branches out of range which no longer is an error.

TomTan added inline comments.Feb 1 2019, 10:52 AM
COFF/Chunks.cpp
690

nit: specify the range is ±4GB?

ruiu accepted this revision.Feb 1 2019, 10:54 AM

LGTM

This revision is now accepted and ready to land.Feb 1 2019, 10:54 AM
This revision was automatically updated to reflect the committed changes.

@ruiu - What do you think about this and D57574 wrt backporting to 8.0? They should be pretty safe, but on the other hand I don't think they're something that many random users will run into either.

ruiu added a comment.Feb 1 2019, 2:40 PM

@ruiu - What do you think about this and D57574 wrt backporting to 8.0? They should be pretty safe, but on the other hand I don't think they're something that many random users will run into either.

I wouldn't personally request cherrypicking that change at this point because it's not a regression from 7.0 and doesn't seem like a big issue. But if you feel otherwise, feel free to submit a cherrypick request.

dmajor added a comment.Feb 1 2019, 7:01 PM

@ruiu - What do you think about this and D57574 wrt backporting to 8.0? They should be pretty safe, but on the other hand I don't think they're something that many random users will run into either.

I wouldn't personally request cherrypicking that change at this point because it's not a regression from 7.0 and doesn't seem like a big issue. But if you feel otherwise, feel free to submit a cherrypick request.

I'd like to have these fixes in the clang 8 used by Mozilla automation. It would make things mildly easier if they were merged to the release branch, but if needed we can carry the patches locally.

ruiu added a comment.Feb 1 2019, 7:45 PM

I'd like to have these fixes in the clang 8 used by Mozilla automation. It would make things mildly easier if they were merged to the release branch, but if needed we can carry the patches locally.

I'm fine if you file a cherrypick request.