Bitcode reader: Inline readAbbreviatedField in readRecord and move the enclosing loop in each case (NFC)

[mehdi_amini]

Summary

This make readRecord 20% faster, measured on an LTO build

[tejohnson]

Nice improvement. Do you know how much this improves the overall parse time or compile time?

lib/Bitcode/Reader/BitstreamReader.cpp

134–152

Similar loop unswitching optimization could be done here, but presumably not as hot. Consider implementing here too for consistency?

225–226

Move these into the default case in your new switch below. Otherwise essentially checking for the error case multiple times.

[mehdi_amini]

Take Teresa's comment into account.

To answer the question on the impact on the total time: my profile shows readRecord for ~3% for an LTO link and close to %10 for a ThinLTO link.

[tejohnson]

LGTM with fix noted below.

lib/Bitcode/Reader/BitstreamReader.cpp

146

ReadVBR64?

[mehdi_amini]

ReadVBR64(), good catch!

[mehdi_amini]

r262811

lib/Bitcode/Reader/BitstreamReader.cpp

if (Op.getEncoding() == BitCodeAbbrevOp::Array) {
       // Array case.  Read the number of elements as a vbr6.
       unsigned NumElts = ReadVBR(6);
 
       // Get the element encoding.
       assert(i+2 == e && "array op not second to last?");
       const BitCodeAbbrevOp &EltEnc = Abbv->getOperandInfo(++i);
 
       // Read all the elements.
+      // Decode the value as we are commanded.
+      switch (EltEnc.getEncoding()) {
+      default:
+        report_fatal_error("Array element type can't be an Array or a Blob");
+      case BitCodeAbbrevOp::Fixed:
+        assert((unsigned)Op.getEncodingData() <= MaxChunkSize);
+        for (; NumElts; --NumElts)
+          Read((unsigned)EltEnc.getEncodingData());
+        break;
+      case BitCodeAbbrevOp::VBR:
+        assert((unsigned)Op.getEncodingData() <= MaxChunkSize);
         for (; NumElts; --NumElts)
-        skipAbbreviatedField(*this, EltEnc);
+          ReadVBR64((unsigned)EltEnc.getEncodingData());
+        break;
+      case BitCodeAbbrevOp::Char6:
+        for (; NumElts; --NumElts)
+          Read(6);
+        break;
+      }
       continue;
     }
 
     assert(Op.getEncoding() == BitCodeAbbrevOp::Blob);
     // Blob case.  Read the number of bytes as a vbr6.
     unsigned NumElts = ReadVBR(6);
     SkipToFourByteBoundary();  // 32-bit alignment
 
if (Op.getEncoding() == BitCodeAbbrevOp::Array) {
       unsigned NumElts = ReadVBR(6);
 
       // Get the element encoding.
       if (i + 2 != e)
         report_fatal_error("Array op not second to last");
       const BitCodeAbbrevOp &EltEnc = Abbv->getOperandInfo(++i);
       if (!EltEnc.isEncoding())
         report_fatal_error(
             "Array element type has to be an encoding of a type");
-      if (EltEnc.getEncoding() == BitCodeAbbrevOp::Array ||
-          EltEnc.getEncoding() == BitCodeAbbrevOp::Blob)
-        report_fatal_error("Array element type can't be an Array or a Blob");
 
       // Read all the elements.
+      switch (EltEnc.getEncoding()) {
+      default:
+        report_fatal_error("Array element type can't be an Array or a Blob");
+      case BitCodeAbbrevOp::Fixed:
+        for (; NumElts; --NumElts)
+          Vals.push_back(Read((unsigned)EltEnc.getEncodingData()));
+        break;
+      case BitCodeAbbrevOp::VBR:
         for (; NumElts; --NumElts)
-        Vals.push_back(readAbbreviatedField(*this, EltEnc));
+          Vals.push_back(ReadVBR64((unsigned)EltEnc.getEncodingData()));
+        break;
+      case BitCodeAbbrevOp::Char6:
+        for (; NumElts; --NumElts)
+          Vals.push_back(BitCodeAbbrevOp::DecodeChar6(Read(6)));
+      }
       continue;
     }
 
     assert(Op.getEncoding() == BitCodeAbbrevOp::Blob);
     // Blob case.  Read the number of bytes as a vbr6.
     unsigned NumElts = ReadVBR(6);
     SkipToFourByteBoundary();  // 32-bit alignment