This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Change DIEnumerator payload type from int64_t to APInt
ClosedPublic

Authored by LemonBoy on May 27 2019, 2:12 AM.

Details

Summary

This allows the representation of arbitrarily large enumeration values.
See https://lists.llvm.org/pipermail/llvm-dev/2017-December/119475.html for context.

Diff Detail

Event Timeline

LemonBoy created this revision.May 27 2019, 2:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2019, 2:12 AM
LemonBoy updated this revision to Diff 201516.May 27 2019, 5:48 AM

Oops, I had attached the wrong .patch file.

andrewrk accepted this revision.May 27 2019, 7:50 AM
andrewrk added a subscriber: andrewrk.

LGTM

This revision is now accepted and ready to land.May 27 2019, 7:50 AM
ormris removed a subscriber: ormris.May 28 2019, 9:21 AM
dblaikie added subscribers: JDevlieghere, probinson.
dblaikie added a subscriber: dblaikie.
aprantl requested changes to this revision.Apr 15 2020, 4:40 PM

Can you add a bitcode upgrade test that contains a .bc file with an old-style < 64bit enumerator produced by trunk llc?

llvm/lib/Bitcode/Reader/MetadataLoader.cpp
1285

Should we use LEB128 encoding here — does that make the parser simpler?

1292

This needs some comments. It's not obvious what is being done here.

This revision now requires changes to proceed.Apr 15 2020, 4:40 PM
aprantl added inline comments.Apr 15 2020, 4:44 PM
llvm/lib/IR/LLVMContextImpl.h
360

Is this needed any more, doesn't the APInt have a sign bit already?

llvm/test/Assembler/DIEnumeratorBig.ll
8

This looks good, though you might be able to shorten this test by just having a bunch of standalone DIEnumerator nodes that are listed in a named MDNode. (See other round trip tests for examples)

aprantl added inline comments.Apr 15 2020, 4:56 PM
llvm/lib/Bitcode/Reader/MetadataLoader.cpp
1285

Alternatively, how do we encode an APInt constant in bitcode otherwise, could we factor out and reuse that code here?

LemonBoy marked an inline comment as done.Apr 16 2020, 3:38 AM
LemonBoy added inline comments.
llvm/lib/Bitcode/Reader/MetadataLoader.cpp
1285

You mean by encoding it as a reference to a LEB128-encoded constant?
I've simply mimicked the way ModuleBitcodeWriter::writeConstants encodes BigInt in 64bit chunks and put them right after the METADATA_ENUMERATOR header to avoid changing the bitstream format too much.

1292

Record[1] is repurposed to hold the bigint bit width (this is quite wasteful as it can be folded into the upper half of Record[0]), the "raw" data follows this header.

I'll add a comment to clarify this point.

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
1554

We can save some space here for the common case of small enumerator values and encode such constants without the IsBigInt flag.

llvm/lib/IR/LLVMContextImpl.h
360

That'd be APSint. I didn't use that to avoid adding more APInt -> APSInt conversions but I can try to convert all the callsites to use the latter if you want.

aprantl added inline comments.Apr 16 2020, 3:32 PM
llvm/lib/Bitcode/Reader/MetadataLoader.cpp
1292

It looks like BitcodeReader.cpp already defines readWideAPInt(). It's probably best to call that here.

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
1561

We could factor the code in line 2365 (from writeConstants) into a emitVariableLengthAPInt() helper and use it here.

LemonBoy updated this revision to Diff 258285.Apr 17 2020, 4:14 AM
LemonBoy marked an inline comment as not done.
  • Added an bc upgrade test
  • Cleaned up some code according to the review comments
  • Cleaned up some test cases
aprantl accepted this revision.Apr 17 2020, 10:43 AM

Thank you! This looks a lot cleaner.

This revision is now accepted and ready to land.Apr 17 2020, 10:43 AM
LemonBoy edited the summary of this revision. (Show Details)Apr 17 2020, 11:55 AM

Since I have no commit access feel free to land this patch if you're satisfied with its current state.

Since I have no commit access feel free to land this patch if you're satisfied with its current state.

I'll simplify some tests before committing.

MaskRay updated this revision to Diff 258542.Apr 18 2020, 12:23 PM

Adjust code and tests

Generate DIEnumerator-10.0.ll.bc with a known release llvm-as 10.0.0

MaskRay accepted this revision.Apr 18 2020, 12:36 PM
MaskRay updated this revision to Diff 258544.Apr 18 2020, 12:37 PM

Simplify tests

MaskRay updated this revision to Diff 258545.Apr 18 2020, 12:45 PM

Simplify tests with a named metadata

MaskRay updated this revision to Diff 258546.Apr 18 2020, 12:48 PM

Hopefully last simplification...

MaskRay retitled this revision from Change DIEnumerator payload type to APInt to [DebugInf] Change DIEnumerator payload type from int64_t to APInt.Apr 18 2020, 12:52 PM
MaskRay edited the summary of this revision. (Show Details)
MaskRay retitled this revision from [DebugInf] Change DIEnumerator payload type from int64_t to APInt to [DebugInfo] Change DIEnumerator payload type from int64_t to APInt.
Harbormaster failed remote builds in B53864: Diff 258545!
Harbormaster failed remote builds in B53865: Diff 258546!
This revision was automatically updated to reflect the committed changes.