This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Store Address's alignment into PointerIntPairs
ClosedPublic

Authored by aeubanks on Jan 13 2022, 4:31 PM.

Details

Summary

This mitigates the extra memory caused by D115725.

Diff Detail

Event Timeline

aeubanks requested review of this revision.Jan 13 2022, 4:31 PM
aeubanks created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2022, 4:31 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rnk added a comment.Jan 13 2022, 5:56 PM

Thanks! Does this have any impact on binary size, as a proxy for more thorough performance testing?

# no asserts, no debuginfo, -O3
$ du -b clang-stable clang-patched
83539528        clang-stable
83528456        clang-patched

clang's code size actually goes down with this patch

llvm-compile-time-tracker:
https://llvm-compile-time-tracker.com/compare.php?from=a9bfb4c4f48d9434918374566d5780c97a805bcc&to=e7e52fd42632a5a8d57520ce5ca2596f7c3be2a1&stat=instructions

there is a little bit of red for compile times, and a little bit of green for max-rss, I think this tradeoff is fine

nikic added inline comments.Jan 18 2022, 1:46 PM
clang/lib/CodeGen/Address.h
37

Are we guaranteed 3 bits even on 32-bit architectures?

91

Why can we assume this?

nikic added inline comments.Jan 18 2022, 1:50 PM
clang/lib/CodeGen/Address.h
91

Ah, the alignment is 64-bits, so the log2 is 0-63, which fits exactly into 6 bits. That's handy :)

aeubanks added inline comments.Jan 18 2022, 9:42 PM
clang/lib/CodeGen/Address.h
37

Apparently not, a static assert fires for a 32-bit build of clang. I was assuming 3 was fine based on the PointerIntPair comments.

/// PointerIntPair - This class implements a pair of a pointer and small
/// integer.  It is designed to represent this in the space required by one
/// pointer by bitmangling the integer into the low part of the pointer.  This
/// can only be done for small integers: typically up to 3 bits, but it depends
/// on the number of bits available according to PointerLikeTypeTraits for the
/// type.

I suppose we could have a separate 32-bit vs 64-bit implementation of Address, although that's not very nice.

dblaikie added inline comments.Jan 18 2022, 11:02 PM
clang/lib/CodeGen/Address.h
37

You can get more spare bits by overaligning the type that you're pointing to, if that's suitable/workable. We do that in a few places in LLVM for this sort of reason.

nikic added inline comments.Jan 19 2022, 12:16 AM
clang/lib/CodeGen/Address.h
37

As we're storing pointers to core LLVM types here (Value and Type), I don't think it's possible to over-align them.

dblaikie added inline comments.Jan 19 2022, 9:33 AM
clang/lib/CodeGen/Address.h
37

Ah, right :/

rnk added inline comments.Jan 24 2022, 4:16 PM
clang/lib/CodeGen/Address.h
37

Oh well, this probably won't work without some truly squirelly hackery. We store four bits of alignment locally, and make all-zeros a sentinel value to store large alignments out of line. It's not worth it.

aeubanks updated this revision to Diff 403070.Jan 25 2022, 4:17 PM

only use PointerIntPair when alignof(Value*) >= 8

nikic added a comment.Jan 26 2022, 5:12 AM

Approach looks reasonable to me.

clang/lib/CodeGen/Address.h
29

Why do we need the extra T parameter?

70

This should probably be 1UL to avoid UB for large alignments.

aeubanks updated this revision to Diff 403323.Jan 26 2022, 9:57 AM

address comments

clang/lib/CodeGen/Address.h
29

without it we end up instantiating AddressImpl<true> unconditionally (because all template parameters are known?) which static_asserts in 32 bit builds

nikic accepted this revision.Jan 26 2022, 10:08 AM

LG from my side

This revision is now accepted and ready to land.Jan 26 2022, 10:08 AM
rnk accepted this revision.Jan 26 2022, 10:32 AM

lgtm

BTW, as a follow-on cleanup, which I may attempt, I think we could convert this alignment from CharUnits to llvm::Align. The vast majority of the getAlignment callers feed it to CGBuilder::CreateAlignedLoad/Store:
http://llvm-cs.pcc.me.uk/tools/clang/lib/CodeGen/Address.h/rgetAlignment
Those call IRBuilder::CreateAlignedLoad/Store, which accept MaybeAlign.

This revision was landed with ongoing or failed builds.Jan 26 2022, 10:39 AM
This revision was automatically updated to reflect the committed changes.
LegalizeAdulthood added inline comments.
clang/lib/CodeGen/Address.h
70

This is causing warnings to be emitted:

clang\lib\CodeGen\Address.h(69,1): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?) [D:\legalize\llvm\llvm-project\build\tools\clang\lib\CodeGen\obj.clangCodeGen.vcxproj]
aeubanks added inline comments.Jan 26 2022, 5:57 PM
clang/lib/CodeGen/Address.h
70

Should be fixed by eee97f1617c94b8