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

711–712

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).

825

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?

2014–2017

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

[sfuniak]

mlir/lib/Rewrite/ByteCode.cpp

711–712

Sure, we can add the filename.

825

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

2014–2015

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

2014

nit: Can you add a comment here?

[sfuniak]

Added comment.

[sfuniak]

Rebased.

mlir/lib/Rewrite/ByteCode.cpp

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()];
     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) {
+    // Print the location of the operation being executed.
+    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: