[PATCH] D54755: [DebugInfo] IR/Bitcode changes for DISubprogram flags

[Paul Robinson via Phabricator via llvm-commits]

probinson marked an inline comment as done.
probinson added a comment.

In D54755#1308216 <https://reviews.llvm.org/D54755#1308216>, @aprantl wrote:

> I'm assuming we already have plenty of DISubprograms with flags in the old format as .bc tests that cover the bitcode upgrade?


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.



================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:1645
   Record.push_back(VE.getMetadataOrNullID(N->getType()));
-  Record.push_back(N->isLocalToUnit());
-  Record.push_back(N->isDefinition());
+  Record.push_back(0); // unused
+  Record.push_back(0); // unused
----------------
aprantl wrote:
> 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.
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.
I'm happy to make that change if you think the size is worthwhile.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54755/new/

https://reviews.llvm.org/D54755