Added line numbers to the debug output of PDL bytecode.

[sfuniak]

Summary

This is a small diff that splits out the debug output for PDL bytecode. When running bytecode with debug output on, it is useful to know the line numbers where the PDLIntepr operations are performed. Usually, these are in a single MLIR file, so it's sufficient to print out the line number rather than the entire location (which tends to be quite verbose). This debug output is gated by LLVM_DEBUG rather than #ifndef NDEBUG to make it easier to test.

[rriddle]

mlir/lib/Rewrite/ByteCode.cpp

704–705

What's the downside to adding in the filename? I've had some thoughts of building a stepthrough debugger for the bytecode and I'd like to have the filename, as the frontend I have (PDLL presented recently) allows include files).

818

Can we move all of these additions to functions/helpers that explicitly spell out what they mean? i.e. when I see the NoOp here it isn't clear to me what it is supposed to be used for, but we actually have a line number being consumed. Also, why do only some operations have this addition, but not others?

1952–1955

Can you move this to a helper function+add documentation? it isn't clear what this corresponds to.

[sfuniak]

mlir/lib/Rewrite/ByteCode.cpp

704–705

Sure, we can add the filename.

818

The requirement would be that, in the debug mode, all pdl_interp operations generate some bytecode operation. So only those operations that did not produce bytecode now have a NoOp. But it's not ideal that with debug output on, we have a different set of bytecode operations. I will change the mechanism to avoid the NoOps.

[sfuniak]

Print location instead of line numbers.

[rriddle]

mlir/lib/Rewrite/ByteCode.cpp

1952–1953

Can we avoid this? We end up paying the cost for every bytecode op. Could we extract the old -1 in executeForEach to something like getPrevCodeIt that adjusts for the location in debug mode?

[sfuniak]

Extract the previous code iterator manually.

[sfuniak]

Rebased.

[rriddle]

LG, thanks!

mlir/lib/Rewrite/ByteCode.cpp

1952

nit: Can you add a comment here?

[sfuniak]

Added comment.

[sfuniak]

Rebased.

mlir/lib/Rewrite/ByteCode.cpp

 
 //===----------------------------------------------------------------------===//
 // Generator
 
 namespace {
 struct ByteCodeLiveRange;
 struct ByteCodeWriter;
 
+/// Check if the given class `T` has an iterator type.
+template <typename T, typename... Args>
+using has_pointer_traits = decltype(std::declval<T>().getAsOpaquePointer());
+
 /// This class represents the main generator for the pattern bytecode.
 class Generator {
 public:
   Generator(MLIRContext *ctx, std::vector<const void *> &uniquedData,
             SmallVectorImpl<ByteCodeField> &matcherByteCode,
             SmallVectorImpl<ByteCodeField> &rewriterByteCode,
             SmallVectorImpl<PDLByteCodePattern> &patterns,
             ByteCodeField &maxValueMemoryIndex,
PDLValue::Kind kind =
                 return PDLValue::Kind::TypeRange;
               return PDLValue::Kind::ValueRange;
             })
             .Case<pdl::TypeType>([](Type) { return PDLValue::Kind::Type; })
             .Case<pdl::ValueType>([](Type) { return PDLValue::Kind::Value; });
     bytecode.push_back(static_cast<ByteCodeField>(kind));
   }
 
-  /// Check if the given class `T` has an iterator type.
-  template <typename T, typename... Args>
-  using has_pointer_traits = decltype(std::declval<T>().getAsOpaquePointer());
-
   /// Append a value that will be stored in a memory slot and not inline within
   /// the bytecode.
   template <typename T>
   std::enable_if_t<llvm::is_detected<has_pointer_traits, T>::value ||
                    std::is_pointer<T>::value>
   append(T value) {
     bytecode.push_back(generator.getMemIndex(value));
   }
struct ByteCodeWriter {
 
   /// Append a variadic number of fields to the bytecode.
   template <typename FieldTy, typename Field2Ty, typename... FieldTys>
   void append(FieldTy field, Field2Ty field2, FieldTys... fields) {
     append(field);
     append(field2, fields...);
   }
 
+  /// Appends a value as a pointer, stored inline within the bytecode.
+  template <typename T>
+  std::enable_if_t<llvm::is_detected<has_pointer_traits, T>::value>
+  appendInline(T value) {
+    constexpr size_t numParts = sizeof(const void *) / sizeof(ByteCodeField);
+    const void *pointer = value.getAsOpaquePointer();
+    ByteCodeField fieldParts[numParts];
+    std::memcpy(fieldParts, &pointer, sizeof(const void *));
+    bytecode.append(fieldParts, fieldParts + numParts);
+  }
+
   /// Successor references in the bytecode that have yet to be resolved.
   DenseMap<Block *, SmallVector<unsigned, 4>> unresolvedSuccessorRefs;
 
   /// The underlying bytecode buffer.
   SmallVectorImpl<ByteCodeField> &bytecode;
 
   /// The main generator producing PDL.
   Generator &generator;
for (Block *block : rpot) {
     // Keep track of where this block begins within the matcher function.
     blockToAddr.try_emplace(block, matcherByteCode.size());
     for (Operation &op : *block)
       generate(&op, writer);
   }
 }
 
 void Generator::generate(Operation *op, ByteCodeWriter &writer) {
+  LLVM_DEBUG({
+    // The following list must contain all the operations that do not
+    // produce any bytecode.
+    if (!isa<pdl_interp::CreateAttributeOp, pdl_interp::CreateTypeOp,
+             pdl_interp::InferredTypesOp>(op))
+      writer.appendInline(op->getLoc());
+  });
   TypeSwitch<Operation *>(op)
       .Case<pdl_interp::ApplyConstraintOp, pdl_interp::ApplyRewriteOp,
             pdl_interp::AreEqualOp, pdl_interp::BranchOp,
             pdl_interp::CheckAttributeOp, pdl_interp::CheckOperandCountOp,
             pdl_interp::CheckOperationNameOp, pdl_interp::CheckResultCountOp,
             pdl_interp::CheckTypeOp, pdl_interp::CheckTypesOp,
             pdl_interp::ContinueOp, pdl_interp::CreateAttributeOp,
             pdl_interp::CreateOperationOp, pdl_interp::CreateTypeOp,

 void Generator::generate(pdl_interp::ContinueOp op, ByteCodeWriter &writer) {
   assert(curLoopLevel > 0 && "encountered pdl_interp.continue at top level");
   writer.append(OpCode::Continue, ByteCodeField(curLoopLevel - 1));
 }
 void Generator::generate(pdl_interp::CreateAttributeOp op,
                          ByteCodeWriter &writer) {
   // Simply repoint the memory index of the result to the constant.
   getMemIndex(op.attribute()) = getMemIndex(op.value());
 }
 void Generator::generate(pdl_interp::CreateOperationOp op,
                          ByteCodeWriter &writer) {
   writer.append(OpCode::CreateOperation, op.operation(),
                 OperationName(op.name(), ctx));
   writer.appendPDLValueList(op.operands());
 
   // Add the attributes.
   OperandRange attributes = op.attributes();
private:
 
   /// Pops a code iterator from the stack, returning true on success.
   void popCodeIt() {
     assert(!resumeCodeIt.empty() && "attempt to pop code off empty stack");
     curCodeIt = resumeCodeIt.back();
     resumeCodeIt.pop_back();
   }
 
+  /// Return the bytecode iterator at the start of the current op code.
+  const ByteCodeField *getPrevCodeIt() const {
+    LLVM_DEBUG({
+      // Account for the op code and the Location stored inline.
+      return curCodeIt - 1 - sizeof(const void *) / sizeof(ByteCodeField);
+    });
+
+    // Account for the op code only.
+    return curCodeIt - 1;
+  }
+
   /// Read a value from the bytecode buffer, optionally skipping a certain
   /// number of prefix values. These methods always update the buffer to point
   /// to the next field after the read data.
   template <typename T = ByteCodeField>
   T read(size_t skipN = 0) {
     curCodeIt += skipN;
     return readImpl<T>();
   }
for (unsigned i = 0, e = read(); i != e; ++i) {
         list.push_back(read<Value>());
       } else {
         ValueRange *values = read<ValueRange *>();
         list.append(values->begin(), values->end());
       }
     }
   }
 
+  /// Read a value stored inline as a pointer.
+  template <typename T>
+  std::enable_if_t<llvm::is_detected<has_pointer_traits, T>::value, T>
+  readInline() {
+    const void *pointer;
+    std::memcpy(&pointer, curCodeIt, sizeof(const void *));
+    curCodeIt += sizeof(const void *) / sizeof(ByteCodeField);
+    return T::getFromOpaquePointer(pointer);
+  }
+
   /// Jump to a specific successor based on a predicate value.
   void selectJump(bool isTrue) { selectJump(size_t(isTrue ? 0 : 1)); }
   /// Jump to a specific successor based on a destination index.
   void selectJump(size_t destIndex) {
     curCodeIt = &code[read<ByteCodeAddr>(destIndex * 2)];
   }
 
   /// Handle a switch operation with the provided value and cases.

 }
 
 void ByteCodeExecutor::executeFinalize() {
   LLVM_DEBUG(llvm::dbgs() << "Executing Finalize\n");
 }
 
 void ByteCodeExecutor::executeForEach() {
   LLVM_DEBUG(llvm::dbgs() << "Executing ForEach:\n");
-  // Subtract 1 for the op code.
-  const ByteCodeField *it = curCodeIt - 1;
+  const ByteCodeField *prevCodeIt = getPrevCodeIt();
   unsigned rangeIndex = read();
   unsigned memIndex = read();
   const void *value = nullptr;
 
   switch (read<PDLValue::Kind>()) {
   case PDLValue::Kind::Operation: {
     unsigned &index = loopIndex[read()];
     const ArrayRef<Operation *> &array = opRangeMemory[rangeIndex];
case PDLValue::Kind::Operation: {
     return;
   }
   default:
     llvm_unreachable("unexpected `ForEach` value kind");
   }
 
   // Store the iterate value and the stack address.
   memory[memIndex] = value;
-  pushCodeIt(it);
+  pushCodeIt(prevCodeIt);
 
   // Skip over the successor (we will enter the body of the loop).
   read<ByteCodeAddr>();
 }
 
 void ByteCodeExecutor::executeGetAttribute() {
   LLVM_DEBUG(llvm::dbgs() << "Executing GetAttribute:\n");
   unsigned memIndex = read();
void ByteCodeExecutor::executeSwitchTypes() {
   });
 }
 
 void ByteCodeExecutor::execute(
     PatternRewriter &rewriter,
     SmallVectorImpl<PDLByteCode::MatchResult> *matches,
     Optional<Location> mainRewriteLoc) {
   while (true) {
+    LLVM_DEBUG(llvm::dbgs() << readInline<Location>() << "\n");
     OpCode opCode = static_cast<OpCode>(read());
     switch (opCode) {
     case ApplyConstraint:
       executeApplyConstraint(rewriter);
       break;
     case ApplyRewrite:
       executeApplyRewrite(rewriter);
       break;
     case AreEqual:
       executeAreEqual();
       break;