This allows the representation of arbitrarily large enumeration values.
See https://lists.llvm.org/pipermail/llvm-dev/2017-December/119475.html for context.
Details
Diff Detail
- Repository
 - rG LLVM Github Monorepo
 
Event Timeline
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.  | |
| 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)  | |
| 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?  | |
| llvm/lib/Bitcode/Reader/MetadataLoader.cpp | ||
|---|---|---|
| 1285 | You mean by encoding it as a reference to a LEB128-encoded constant?  | |
| 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.  | |
| 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.  | |
- Added an bc upgrade test
 - Cleaned up some code according to the review comments
 - Cleaned up some test cases
 
Since I have no commit access feel free to land this patch if you're satisfied with its current state.
Adjust code and tests
Generate DIEnumerator-10.0.ll.bc with a known release llvm-as 10.0.0
Should we use LEB128 encoding here — does that make the parser simpler?