Packing the flags into one bitcode word will simplify adding new flags in the future.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
I'm assuming we already have plenty of DISubprograms with flags in the old format as .bc tests that cover the bitcode upgrade?
llvm/lib/Bitcode/Writer/BitcodeWriter.cpp | ||
---|---|---|
1645 ↗ | (On Diff #174773) | Am I right in assuming that these dead fields take up virtually no extra space in the bitcode? Otherwise we might want to add an extra bit distinguish old vs. a newer, more compact record layout. |
llvm/lib/IR/DebugInfoMetadata.cpp | ||
554 ↗ | (On Diff #174773) | nit: comment should go on the line before. |
That was my thinking, but perhaps it would do no harm to add another specifically for this. Basically it would be a pre-patch Assembler/disubprogram.ll run through a pre-patch llvm-as which the test would run through llvm-dis and verify the flags came out right.
llvm/lib/Bitcode/Writer/BitcodeWriter.cpp | ||
---|---|---|
1645 ↗ | (On Diff #174773) | Each field in Record is 64-bit, so as written this is wasting 24 bytes per DISubprogram in the bitcode. Compressing those out would mean the reader would pretty much have to handle the old/new formats separately, as the indexes of all fields after this one would vary too much to handle cleanly any other way. |
llvm/lib/Bitcode/Writer/BitcodeWriter.cpp | ||
---|---|---|
1645 ↗ | (On Diff #174773) | I don't /think/ that's how the bitcode format works, though, right? I believe it's a more compact encoding than using all 64 bits regardless of the value encoded... but I don't know the specifics/not very sure about it. |
llvm/lib/Bitcode/Writer/BitcodeWriter.cpp | ||
---|---|---|
1645 ↗ | (On Diff #174773) | @dblaikie should be right. IIRC the default (in absence of an explicit abbreviation) is vbr-6, so that's 6 bits times three for 18 bits. That said, I'm not convinced this would be hard to deal with in the reader. There's probably a way to reorganize the logic to avoid forking. |
Don't emit unused fields to the bitcode. Accommodate the new variant in the reader.
This does not address all comments, but I wanted to show @dexonsmith the change for bitcode format that he asked for.
I think it would be nice to have one small explicit "DISubprogram version 4" .bc upgrade test, otherwise this LGTM now. Thanks!
Oops, I had meant to respond to this one:
This does not address all comments, but I wanted to show @dexonsmith the change for bitcode format that he asked for.
While I was updating some internal testcases I wondered: why is DIFlagPrototype not a DISPFlag? Does this flag make sense on something other than functions?
Because I was not interested in moving existing flags from the old word to the new word in the first round. There are others that probably apply only to subprograms:
NoReturn, MainSubprogram; probably Thunk, All CallsDescribed; maybe Trivial, Explicit.
Really we should do some kind of audit, and do a bulk move in one go so we can minimize the bitcode-upgrade pain.
Thanks--by clever choice of where to place the new field, it was possible to minimize the variants. I'm not super happy about having to track two different offsets, but it's not much worse than the old code.
One additional problem is that the existing flags are exposed as part of the public API, i.e. to front-ends, so we'll need to work out how to avoid Bad Stuff(tm) if we repurpose any existing flag bits.
Another option here is not to merge them at all in bitcode (I didn't consider this before). You can add an abbreviation for a fixed-width field of 1 bit each:
https://llvm.org/docs/BitCodeFormat.html#fixed-width-value
Wouldn't that cause every new flag to be a bitcode format change? The value of having a flags word is that new flags in the same word don't affect the bitcode format.