This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Add LOH_ARM64_ADRP_LDR_GOT_LDR optimization hint support
ClosedPublic

Authored by BertalanD on Jul 9 2022, 12:29 PM.

Details

Summary

[lld-macho] Add LOH_ARM64_ADRP_LDR_GOT_LDR optimization hint support

This hint relaxes a load from an address specified in the GOT. If the
symbol is external and the GOT is within a +/- 1 MiB range, the adrp +
add used for loading the GOT is turned into adr + nop. If the symbol is
local, the adrp + add + ldr sequence is turned into a literal ldr or
adr(p) + ldr if possible.

This type accounts for more than half of all LOHs in chromium_framework.

This commit moves the eligibility checks into helper functions to
improve the readability of the LOH processing code. Ho functional
changes are intended to the previously implemented LOH types.

Diff Detail

Event Timeline

BertalanD created this revision.Jul 9 2022, 12:29 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 9 2022, 12:29 PM
BertalanD requested review of this revision.Jul 9 2022, 12:29 PM

The conditions in the if and else if hurt my eyes. Maybe comments could help your reviewers and future readers? Maybe hoist the 1 << 20 as a constant up?

BertalanD updated this revision to Diff 443665.Jul 11 2022, 9:04 AM
BertalanD edited the summary of this revision. (Show Details)

The conditions in the if and else if hurt my eyes

That's a bit harsh, but true. I moved the range checks into helper functions, and the code looks a lot more readable IMO.

The conditions in the if and else if hurt my eyes

That's a bit harsh, but true. I moved the range checks into helper functions, and the code looks a lot more readable IMO.

Sorry, that was not meant to offensive.

int3 added a subscriber: int3.Jul 11 2022, 6:47 PM
int3 added inline comments.
lld/MachO/Arch/ARM64.cpp
257–259

MathExtras.h has isInt -- I think we could just call that inline (i.e. don't need this function)

268–269

I know Ldr is small enough to be copyable but personally I'd just pass by const ref here so we don't have to think about that

269

can this just be an assert?

270–271

MathExtras has isShiftedInt

(also consider calling that inline; I'm not sure the sizeValid check / assert above is really necessary)

278

could use maskTrailingOnes instead of the hex constant

296

so this is checking specifically for the unsigned variant of the instruction, right? I assume we don't care about the signed variants because we generally expect the offset to be positive? either way, would be nice to explain in a comment :)

298

delta3? you mean ldr.offset?

300

might have mentioned it in a previous diff, but I would love if we could store p2Size instead of size (i.e. store the power-of-two exponent value). Would let us skip the relatively expensive div/mod operations + writing in bitshifts makes it clearer what the encoding constraints are.

(not gonna block the diff for this though)

300–301

parens unnecessary (I get that it is hard to remember operator precedence in general, but I think it's easy enough to remember that non-bitwise math operations are always higher prec than equality checks, and that the short-circuiting operators are lower prec than both)

320

I think we could avoid the cast if we declared enum ExtendType : uint8_t

BertalanD updated this revision to Diff 443960.Jul 12 2022, 8:40 AM

Use helper functions from MathExtras.h, store the log2 of load sizes, remove a couple of redundant checks.

BertalanD marked 7 inline comments as done.Jul 12 2022, 8:43 AM
BertalanD added inline comments.
lld/MachO/Arch/ARM64.cpp
257–259

Thank you for telling me about the MathExtras.h functions, I didn't know they existed. I updated the diff to use those, but I'd like to keep the helper functions for readability.

269

Currently, this function *will* be called with ldr.size == 1 and ldr.size == 2, and we fall back to the ADR+LDR transformation if that happens (see line 150 of the test)

int3 accepted this revision.Jul 12 2022, 10:11 AM

lgtm, thanks!

lld/MachO/Arch/ARM64.cpp
278

missed this one

296

Thanks for the comment. Am I right to say that the offsets that come in here are usually positive, though? Or is that not true (just curious)

This revision is now accepted and ready to land.Jul 12 2022, 10:11 AM
BertalanD added inline comments.Jul 12 2022, 11:24 AM
lld/MachO/Arch/ARM64.cpp
296

I think you're right. This might actually *never* be negative in normal circumstances. We're transforming

adrp x0, _foo@PAGE
add  x1, x0, _foo@PAGEOFF
ldr  x2, [x1, #off]

into

adrp x0, _foo@PAGE
nop
ldr x2, [x0, _foo@PAGEOFF + #off]

#off cannot be negative because the third instruction of a well-formed AdrpLdrGotLdr sequence must be an LDRXui or LDRWui which have an unsigned offset, and _foo@PAGEOFF is evidently a positive value.

BertalanD added inline comments.Jul 12 2022, 12:58 PM
lld/MachO/Arch/ARM64.cpp
278

Would that look something like this?

uint32_t imm19 = (ldr.offset & ~maskTrailingOnes<uint32_t>(2)) << 3;

Honestly, that doesn't look more readable to me.

int3 added inline comments.Jul 12 2022, 2:15 PM
lld/MachO/Arch/ARM64.cpp
278

I think it is

uint32_t imm19 = (ldr.offset & maskTrailingOnes<uint32_t>(19)) << 3;

Having to specify the template param is a bit unfortunate, but mainly I think it's nice to have the 19 explicit there, rather than having to parse that from the hex constant. Alternatively (1 << 20) - 1 would be similarly explicit.

Not a big deal though, feel free to keep the hex constant if you prefer

296

That's what I thought. Thanks for confirming :)

BertalanD added inline comments.Jul 12 2022, 3:37 PM
lld/MachO/Arch/ARM64.cpp
278

Wouldn't it be this instead?

uint32_t imm19 = (ldr.offset & maskTrailingOnes<uint32_t>(21)) << 3;

The 5 least significant bits encode the destination register and the next 19 bits the offset divided by 4. We assert that the offset is a multiple of 4, so after the shift, the 5 LSB will be all zeros.

int3 added inline comments.Jul 12 2022, 5:22 PM
lld/MachO/Arch/ARM64.cpp
278

d'oh, of course :) thanks

Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2022, 3:23 AM