[MooreToCore] Crash when getting values to observe

[maerhart]

The following crashes due to an unattached region when calling getValuesToObserve.

moore.module @crash(in %in0: !moore.i32, in %in1: !moore.i32) {
  %var = moore.variable : <!moore.i32>
  moore.procedure always_comb {
    %0 = moore.pows %in0, %in1 : !moore.i32
    moore.blocking_assign %var, %0 : !moore.i32
    moore.return
  }
}

[hailongSun2000]

As far as I analyzed preliminarily, in0 and in1 are block arguments, and then we'll get the <<UNLINKED BLOCK>> when executing value->getParentRegion()
Appendix:

/// llvm/mlir/lib/IR/Value.cpp:41

/// Return the Region in which this Value is defined.
Region *Value::getParentRegion() {
  if (auto *op = getDefiningOp())
    return op->getParentRegion();
  return llvm::cast<BlockArgument>(*this).getOwner()->getParent();
}

llvm::cast<BlockArgument>(*this).getOwner() returns UNLINKED BLOCK.

Do we need to add a check for this 🤔 ? Normally, we can get the correct Owner or DefiningOp when translating verilog into Moore IR, right?

[maerhart]

I'm not really sure if it's enough to just check if the region is attached. Can we be sure that if the region is detached the value is defined outside the process or not necessarily?

We may have to analyze and cache the list of values to observe at every wait_event before calling the dialect converter, and pass a reference to this cache to the conversion pattern to use it there. The rewriter has a getRemappedValue method to get the value in the target IR for each of the values in the cache. However, it may return a NULL value if the defining operation was not processed yet, so if the value is defined after the procedure that might be the case. Maybe use `materializeTargetConversion' instead?

If we can't get things working reliably inside a pattern we might have to move the procedure lowering outside the dialect conversion (lowering the procedure boundaries before the dialect conversion).

[hailongSun2000]

Like this 🤔 ?

moore.module @crash(in %in0: !moore.i32, in %in1: !moore.i32) {
  %var = moore.variable : <!moore.i32>
  moore.procedure always_comb {
    %0 = moore.read %a : <!moore.i32>
    %1 = moore.read %in0_0 : <i32>
    %2 = moore.pows %0, %1 : !moore.i32
    moore.blocking_assign %var, %2 : !moore.i32
    moore.return
  }
  // variable declaration
  %a = moore.variable : <!moore.i32>
  // declaration for every port used in the module body.
  %in0_0 = moore.variable name "in0" : <i32>
}

The above case can work. Let me think of other long-tail cases.

We can ensure that if any of the variables have DefiningOp, value->getParentRegion() will always return non-nullprt.

[hailongSun2000]

I'm curious why getOwner() returns UNLINKED BLOCK. In my opinion, %in0 is the block argument of the module crash, so based on the annotation of getOwner(), its parent operation should be the module crash, rather than UNLINKED 🤔 .

[hailongSun2000]

Oh! Hey, @maerhart! I think I found the answer.
If we add value = rewriter.getRemappedValue(value); before if (region->isAncestor(value.getParentRegion())), it can work.
The block argument list of moore.module has been inline into hw.module when executing SVModuleConversion. However, the value from operation->getOperands() is still before conversion. I hope you can realize what my confusing words express.

[maerhart]

Ah yes, that's a great observation! I think that's the right fix for this issue. Can you open a PR for that?