Address code review feedback

Make the GCInfo logs more compact

include/GcInfo/GcInfo.h

@@ -16,6 +16,7 @@
 #ifndef GCINFO_H
 #define GCINFO_H
 
+#include "gcinfoencoder.h"
 #include "jitpch.h"
 #include "LLILCJit.h"
 
@@ -27,26 +28,35 @@ class GcInfoEncoder;
 class GCInfo {
 public:
   /// Construct a GCInfo object
-  /// \param JitContext Context record for the method's jit request.
+  /// \param JitCtx Context record for the method's jit request.
   /// \param StackMapData A pointer to the .llvm_stackmaps section
   ///        loaded in memory
-  /// \param Start address of the Code section block
-  GCInfo(LLILCJitContext *JitContext, uint8_t *LLVMStackMapData,
-         uint8_t *CodeBlockStart);
+  /// \param CodeBlkStart Start address of the Code section block
+  GCInfo(LLILCJitContext *JitCtx, uint8_t *StackMapData, uint8_t *CodeBlkStart,
+         GcInfoAllocator *Allocator);
 
   /// Emit GC Info to the EE using GcInfoEncoder.
   void emitGCInfo();
 
+  /// Destructor -- delete allocated memory
+  ~GCInfo();
+
 private:
   void encodeHeader();
   void encodeLiveness();
   void emitEncoding();
 
-  LLILCJitContext *JitContext;
-  uint8_t *LLVMStackMapData;
-  GcInfoEncoder *Encoder;
-  uint8_t *CodeBlockStart;
+  const LLILCJitContext *JitContext;
+  const uint8_t *CodeBlockStart;
+  const uint8_t *LLVMStackMapData;
+  GcInfoEncoder Encoder;
   bool EmitLogs;
+
+#if defined(PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED)
+  size_t NumCallSites;
+  unsigned *CallSites;
+  BYTE *CallSiteSizes;
+#endif // defined(PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED)
 };
 
 #endif // GCINFO_H

include/GcInfo/GcInfoUtil.h

@@ -503,10 +503,10 @@ class StructArrayListBase {
   };
   friend class ArrayIteratorBase;
 
-  StructArrayListEntryBase
-      *m_pChunkListHead; // actually StructArrayListEntry<ELEMENT_TYPE>*
-  StructArrayListEntryBase
-      *m_pChunkListTail; // actually StructArrayListEntry<ELEMENT_TYPE>*
+  StructArrayListEntryBase *
+    m_pChunkListHead; // actually StructArrayListEntry<ELEMENT_TYPE>*
+  StructArrayListEntryBase *
+    m_pChunkListTail; // actually StructArrayListEntry<ELEMENT_TYPE>*
   SIZE_T m_nItemsInLastChunk;
   SIZE_T m_nTotalItems;
   SIZE_T m_nLastChunkCapacity;

lib/GcInfo/GcInfo.cpp

@@ -15,30 +15,26 @@
 
 #include "earlyincludes.h"
 #include "GcInfo.h"
-#include "gcinfoencoder.h"
-#include "jitpch.h"
-#include "LLILCJit.h"
 #include "Target.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallBitVector.h"
 #include "llvm/Object/StackMapParser.h"
-#include <map>
-#include <vector>
 #include <sstream>
 
 using namespace llvm;
-using namespace std;
-
-GCInfo::GCInfo(LLILCJitContext *JitContext, uint8_t *LLVMStackMapData,
-               uint8_t *CodeBlockStart) {
-  this->JitContext = JitContext;
-  this->LLVMStackMapData = LLVMStackMapData;
-  this->CodeBlockStart = CodeBlockStart;
-
-  this->Encoder = new GcInfoEncoder(JitContext->JitInfo, JitContext->MethodInfo,
-                                    new GcInfoAllocator());
 
+GCInfo::GCInfo(LLILCJitContext *JitCtx, uint8_t *StackMapData,
+               uint8_t *CodeBlkStart, GcInfoAllocator *Allocator)
+    : JitContext(JitCtx), LLVMStackMapData(StackMapData),
+      CodeBlockStart(CodeBlkStart),
+      Encoder(JitContext->JitInfo, JitContext->MethodInfo, Allocator) {
 #if !defined(NDEBUG)
   this->EmitLogs = JitContext->Options->LogGcInfo;
 #endif // !NDEBUG
+#if defined(PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED)
+  this->CallSites = nullptr;
+  this->CallSiteSizes = nullptr;
+#endif // defined(PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED)
 }
 
 void GCInfo::encodeHeader() {
@@ -51,18 +47,22 @@ void GCInfo::encodeHeader() {
   // https://github.com/dotnet/llilc/issues/679
   // JitContext->HotCodeSize is the size of the allocated code block.
   // It is not the actual length of the current function's code.
-  Encoder->SetCodeLength(JitContext->HotCodeSize);
-  Encoder->SetSizeOfStackOutgoingAndScratchArea(0);
+  Encoder.SetCodeLength(JitContext->HotCodeSize);
+
+#if defined(FIXED_STACK_PARAMETER_SCRATCH_AREA)
+  // TODO: Set size of outgoing/scratch area accurately
+  // https://github.com/dotnet/llilc/issues/681
+  Encoder.SetSizeOfStackOutgoingAndScratchArea(0);
+#endif // defined(FIXED_STACK_PARAMETER_SCRATCH_AREA)
 }
 
 void GCInfo::encodeLiveness() {
   if (LLVMStackMapData == nullptr) {
     return;
   }
 
-  ArrayRef<uint8_t> StackMapContentsArray(
-      reinterpret_cast<const uint8_t *>(LLVMStackMapData),
-      JitContext->StackMapSize);
+  ArrayRef<uint8_t> StackMapContentsArray(LLVMStackMapData,
+                                          JitContext->StackMapSize);
 
 #if defined(BIGENDIAN)
   typedef StackMapV1Parser<support::big> StackMapParserType;
@@ -87,48 +87,52 @@ void GCInfo::encodeLiveness() {
 
   size_t OffsetCorrection = FunctionEntry - CodeBlockStart;
 
-  // Loop over LLVM StackMap records to:
-  // 1) Note CallSites (safepoints)
-  // 2) Assign Slot-IDs to each unique gc-pointer location (slot)
-  // 3) Record liveness (birth/death) of slots per call-site.
+// Loop over LLVM StackMap records to:
+// 1) Note CallSites (safepoints)
+// 2) Assign Slot-IDs to each unique gc-pointer location (slot)
+// 3) Record liveness (birth/death) of slots per call-site.
+
+#if defined(PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED)
+  NumCallSites = StackMapParser.getNumRecords();
+  CallSites = new unsigned[NumCallSites];
+  CallSiteSizes = new BYTE[NumCallSites];
+
+  // TODO: Determine call-site-size accurately
+  // https://github.com/Microsoft/llvm/issues/56
+  // Call-site size is not yet available in LLVM's StackMap,
+  // so make up a value for now.
+  // The Call-instruction generated by LLILC on X86/X64 is typically
+  // Call [rax], which has a two-byte encoding.
+  const uint8_t CallSiteSize = 2;
+
+#endif // defined(PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED)
 
-  size_t NumCallSites = StackMapParser.getNumRecords();
-  unsigned *CallSites = new unsigned[NumCallSites];
-  BYTE *CallSiteSizes = new BYTE[NumCallSites];
-  map<int32_t, GcSlotId> SlotMap;
+  DenseMap<int32_t, GcSlotId> SlotMap;
   size_t NumSlots = 0;
 
   // LLVM StackMap records all live-pointers per Safepoint, whereas
   // CoreCLR's GCTables record pointer birth/deaths per Safepoint.
   // So, we do the translation using old/new live-pointer-sets
-  //
-  // We need bit-sets for recording the liveness -- one bit per slot.
-  // But std::BitSet only supports fixed size bitsets, so we use
-  // vector<bool> which should be optimized by the compiler to use
-  // a bit-wide representation.
+  // using bit-sets for recording the liveness -- one bit per slot.
 
-  vector<bool> OldLiveSet;
-  vector<bool> NewLiveSet;
+  size_t LiveBitSetSize = 25;
+  SmallBitVector OldLiveSet(LiveBitSetSize);
+  SmallBitVector NewLiveSet(LiveBitSetSize);
 
   // TODO: Identify Object and Managed pointers differently
   // https://github.com/dotnet/llilc/issues/28
   // We currently conservatively describe all slots as containing
   // interior pointers
-  GcSlotFlags SlotFlags = (GcSlotFlags)GC_SLOT_INTERIOR;
-
-  // TODO: Determine call-site-size accurately
-  // https://github.com/Microsoft/llvm/issues/56
-  // Call-site size is not yet available in LLVM's StackMap,
-  // so make up a value for now.
-  // The Call-instruction generated by LLILC on X86/X64 is typically
-  // Call [rax], which has a two-byte encoding.
-  uint8_t CallSiteSize = 2;
+  const GcSlotFlags SlotFlags = (GcSlotFlags)GC_SLOT_INTERIOR;
 
 #if !defined(NDEBUG)
   if (EmitLogs) {
     dbgs() << "  FunctionEntry: " << FunctionEntry << "\n"
-           << "  #Safepoints: " << NumCallSites << "\n";
+           << "  #Safepoints: " << StackMapParser.getNumRecords() << "\n";
   }
+
+  std::ostringstream SlotStream;
+  std::ostringstream LiveStream;
 #endif // !NDEBUG
 
   size_t RecordIndex = 0;
@@ -143,18 +147,14 @@ void GCInfo::encodeLiveness() {
     unsigned InstructionOffset =
         R.getInstructionOffset() + OffsetCorrection - 2;
 
+#if defined(PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED)
     CallSites[RecordIndex] = InstructionOffset;
-    CallSiteSizes[RecordIndex] = 2;
+    CallSiteSizes[RecordIndex] = CallSiteSize;
+#endif // defined(PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED)
 
 #if !defined(NDEBUG)
-    ostringstream SlotStream;
-    ostringstream LiveStream;
-
     if (EmitLogs) {
-      LiveStream << "Safepoint: " << RecordIndex << ": ("
-                 << CallSites[RecordIndex] << " - "
-                 << (CallSites[RecordIndex] + CallSiteSizes[RecordIndex])
-                 << ")\n";
+      LiveStream << "    " << RecordIndex << ": @" << InstructionOffset;
     }
 #endif // !NDEBUG
 
@@ -177,7 +177,6 @@ void GCInfo::encodeLiveness() {
         break;
 
       case StackMapParserType::LocationKind::Direct: {
-
         uint16_t DwReg = Loc.getDwarfRegNum();
         switch (DwReg) {
         case DW_FRAME_POINTER:
@@ -194,22 +193,30 @@ void GCInfo::encodeLiveness() {
 
         GcSlotId SlotID;
         int32_t Offset = Loc.getOffset();
-        map<int32_t, GcSlotId>::iterator ExistingSlot = SlotMap.find(Offset);
+        DenseMap<int32_t, GcSlotId>::const_iterator ExistingSlot =
+            SlotMap.find(Offset);
         if (ExistingSlot == SlotMap.end()) {
-          SlotID = Encoder->GetStackSlotId(Offset, SlotFlags, SpBase);
-
+          SlotID = Encoder.GetStackSlotId(Offset, SlotFlags, SpBase);
           SlotMap[Offset] = SlotID;
 
-          // Make space for another slot in the Lifetime trackers.
+          assert(SlotID == NumSlots && "SlotIDs dis-contiguous");
           NumSlots++;
-          OldLiveSet.push_back(false);
-          NewLiveSet.push_back(false);
+
+          if (NumSlots > LiveBitSetSize) {
+            LiveBitSetSize += LiveBitSetSize;
+
+            assert(LiveBitSetSize > OldLiveSet.size() &&
+                   "Overflow -- Too many live pointers");
+
+            OldLiveSet.resize(LiveBitSetSize);
+            NewLiveSet.resize(LiveBitSetSize);
+          }
 
 #if !defined(NDEBUG)
           if (EmitLogs) {
-            SlotStream << "  [" << SlotID
-                       << "]:" << ((SpBase == GC_SP_REL) ? "sp+" : "fp+")
-                       << Offset;
+            SlotStream << "    [" << SlotID
+                       << "]: " << ((SpBase == GC_SP_REL) ? "sp+" : "fp+")
+                       << Offset << "\n";
           }
 #endif // !NDEBUG
         } else {
@@ -219,49 +226,69 @@ void GCInfo::encodeLiveness() {
         NewLiveSet[SlotID] = true;
         break;
       }
+
+      default:
+        assert(false && "Unexpected Location Type");
+        break;
       }
     }
 
     for (GcSlotId SlotID = 0; SlotID < NumSlots; SlotID++) {
       if (!OldLiveSet[SlotID] && NewLiveSet[SlotID]) {
 #if !defined(NDEBUG)
         if (EmitLogs) {
-          LiveStream << "  Live:" << SlotID;
+          LiveStream << "  +" << SlotID;
         }
 #endif // !NDEBUG
-        Encoder->SetSlotState(InstructionOffset, SlotID, GC_SLOT_LIVE);
+        Encoder.SetSlotState(InstructionOffset, SlotID, GC_SLOT_LIVE);
       } else if (OldLiveSet[SlotID] && !NewLiveSet[SlotID]) {
 #if !defined(NDEBUG)
         if (EmitLogs) {
-          LiveStream << "  Dead:" << SlotID;
+          LiveStream << "  -" << SlotID;
         }
 #endif // !NDEBUG
-        Encoder->SetSlotState(InstructionOffset, SlotID, GC_SLOT_DEAD);
+        Encoder.SetSlotState(InstructionOffset, SlotID, GC_SLOT_DEAD);
       }
 
       OldLiveSet[SlotID] = NewLiveSet[SlotID];
       NewLiveSet[SlotID] = false;
     }
 
+    RecordIndex++;
+
 #if !defined(NDEBUG)
     if (EmitLogs) {
-      dbgs() << "  Slots: " << SlotStream.str() << "\n";
-      dbgs() << "  Liveness: " << LiveStream.str() << "\n";
+      LiveStream << "\n";
     }
 #endif // !NDEBUG
+  }
 
-    RecordIndex++;
+#if !defined(NDEBUG)
+  if (EmitLogs) {
+    dbgs() << "  Slots:\n" << SlotStream.str();
+    dbgs() << "  Safepoints:\n" << LiveStream.str() << "\n";
   }
+#endif // !NDEBUG
+
   // Finalize Slot IDs to enable compact representation
-  Encoder->FinalizeSlotIds();
+  Encoder.FinalizeSlotIds();
 
+#if defined(PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED)
   // Encode Call-sites
-  Encoder->DefineCallSites(CallSites, CallSiteSizes, NumCallSites);
+  Encoder.DefineCallSites(CallSites, CallSiteSizes, NumCallSites);
+#endif // defined(PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED)
 }
 
 void GCInfo::emitEncoding() {
-  Encoder->Build();
-  Encoder->Emit();
+  Encoder.Build();
+  Encoder.Emit();
+}
+
+GCInfo::~GCInfo() {
+#if defined(PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED)
+  delete CallSites;
+  delete CallSiteSizes;
+#endif // defined(PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED)
 }
 
 void GCInfo::emitGCInfo() {

lib/Jit/CMakeLists.txt

@@ -54,6 +54,8 @@ else()
   set(LLILC_TARGET_TRIPLE "${LLVM_DEFAULT_TARGET_TRIPLE}")
 endif()
 
+add_definitions(-DSTANDALONE_BUILD)
+
 message(STATUS "LLILC_TARGET_TRIPLE is ${LLILC_TARGET_TRIPLE}")
 add_definitions(-DLLILC_TARGET_TRIPLE="${LLILC_TARGET_TRIPLE}")
 

lib/Jit/LLILCJit.cpp

@@ -288,7 +288,9 @@ CorJitResult LLILCJit::compileMethod(ICorJitInfo *JitInfo,
       // TODO: ColdCodeSize, or separated code, is not enabled or included.
       *NativeSizeOfCode = Context.HotCodeSize + Context.ReadOnlyDataSize;
 
-      GCInfo GcInfo(&Context, MM.getStackMapSection(), MM.getHotCodeBlock());
+      GcInfoAllocator GcInfoAllocator;
+      GCInfo GcInfo(&Context, MM.getStackMapSection(), MM.getHotCodeBlock(),
+                    &GcInfoAllocator);
       GcInfo.emitGCInfo();
 
       // Dump out any enabled timing info.