[PATCH] D62475: Change DIEnumerator payload type to APInt

[l via Phabricator via llvm-commits]

LemonBoy marked an inline comment as done.
LemonBoy added inline comments.


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:1213
     bool IsUnsigned = Record[0] & 2;
+    bool IsBigInt = Record[0] & 4;
+    APInt Value;
----------------
aprantl wrote:
> aprantl wrote:
> > Should we use LEB128 encoding here — does that make the parser simpler?
> Alternatively, how do we encode an APInt constant in bitcode otherwise, could we factor out and reuse that code here?
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.


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:1220
+      SmallVector<uint64_t, 8> Words(NumWords);
+      transform(makeArrayRef(&Record[3], NumWords), Words.begin(),
+                decodeSignRotatedValue);
----------------
aprantl wrote:
> This needs some comments. It's not obvious what is being done here.
`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.


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:1507
+  const uint64_t IsBigInt = 1 << 2;
+  Record.push_back(IsBigInt | (N->isUnsigned() << 1) | N->isDistinct());
+  Record.push_back(N->getValue().getBitWidth());
----------------
We can save some space here for the common case of small enumerator values and encode such constants without the `IsBigInt` flag.


================
Comment at: llvm/lib/IR/LLVMContextImpl.h:360
   MDString *Name;
   bool IsUnsigned;
 
----------------
aprantl wrote:
> Is this needed any more, doesn't the APInt have a sign bit already?
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62475