[PATCH] D150371: TargetExtType: Use a schema-based abbreviation in bitcode

[Nicolai Hähnle via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Bitstream/Reader/BitstreamReader.cpp:380-385
+      size_t NumVals = (size_t)NumElts * NumFields;
+      if (!isSizePlausible(NumVals))
+        return error("Size is not plausible");
+      Vals.reserve(Vals.size() + NumVals);
+
+      Vals.push_back(NumElts);
----------------
Flakebi wrote:
> The code in `BitstreamCursor::skipRecord` and in `BitstreamCursor::readRecord` is almost identical. Maybe it makes sense to move that into a function that takes an optional `Vals` vector?
Yeah, I was wondering the same thing. I suspect that the initial theory was that `skipRecord` can be faster. It's unclear how valid that theory is with modern branch predictors. And if it genuinely is an issue, we could still have a single implementation that we force to be specialized by writing it as a template. In any case, I'd rather do that in a separate change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150371