Skip to content

Conversation

@fabianschuiki
Copy link
Contributor

Add a RefType to the LLHD dialect and replace all uses of HW's InOutType across the LLHD dialect and the ImportVerilog pipeline. We have been abusing the inout type to mean "signal/variable reference", which becomes problematic if we are trying to place !llhd.time or !llvm.ptr values behind such a reference. (The HW inout type requires the inner type to be a plain hardware value.)

This change now allows the circt-verilog frontend to generate variables with the time type and treat them just like any other type.

Most of the changes here are just systematic name replacements. I have also removed a few qualified(...) wrappers around types and made sure that when an op is known to accept or return a ref type, the printer would just print <T> instead of !llhd.ref<T>. This is the default behavior for unambiguous types, but sometimes this gets lost when explicitly askign for qualified types, or using functional-type.

Base automatically changed from fschuiki/llhd-dce to main October 10, 2025 20:12
Copy link
Contributor

@Scheremo Scheremo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM. Quite a bit of churn, but very sensible changes imo!
Lots of doc comment in LLHD Op table gen files still refer to operators returning hw.inout types. Would be nice to update 😃

Comment on lines 1826 to +1828
typeConverter.addConversion([&](RefType type) -> std::optional<Type> {
if (auto innerType = typeConverter.convertType(type.getNestedType())) {
if (hw::isHWValueType(innerType))
return hw::InOutType::get(innerType);
// TODO: There is some abstraction missing here to correctly return a
// reference of a CHandle; return an error for now.
if (isa<mlir::LLVM::LLVMPointerType>(innerType)) {
mlir::emitError(mlir::UnknownLoc::get(type.getContext()))
<< "Emission of references of LLVMPointerType is currently "
"unsupported!";
return {};
}
}
if (auto innerType = typeConverter.convertType(type.getNestedType()))
return llhd::RefType::get(innerType);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much nicer!

Comment on lines +485 to +497
// CHECK: [[TMP:%.+]] = llvm.mlir.zero : !llvm.ptr
// CHECK: llhd.sig [[TMP]] : !llvm.ptr
moore.variable : <!moore.chandle>

// CHECK: [[TMP:%.+]] = llhd.constant_time <0
// CHECK: llhd.sig [[TMP]] : !llhd.time
moore.variable : <!moore.time>

// CHECK: [[TMP:%.+]] = llhd.constant_time
// CHECK: llhd.sig [[TMP]] : !llhd.time
%c42_fs = moore.constant_time 42 fs
moore.variable %c42_fs : <!moore.time>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love to see this!

Add a `RefType` to the LLHD dialect and replace all uses of HW's
`InOutType` across the LLHD dialect and the ImportVerilog pipeline. We
have been abusing the inout type to mean "signal/variable reference",
which becomes problematic if we are trying to place `!llhd.time` or
`!llvm.ptr` values behind such a reference. (The HW inout type requires
the inner type to be a plain hardware value.)

This change now allows the circt-verilog frontend to generate variables
with the `time` type and treat them just like any other type.

Most of the changes here are just systematic name replacements. I have
also removed a few `qualified(...)` wrappers around types and made sure
that when an op is known to accept or return a ref type, the printer
would just print `<T>` instead of `!llhd.ref<T>`. This is the default
behavior for unambiguous types, but sometimes this gets lost when
explicitly askign for qualified types, or using `functional-type`.
@fabianschuiki fabianschuiki merged commit b3ca409 into main Oct 12, 2025
6 checks passed
@fabianschuiki fabianschuiki deleted the fschuiki/llhd-ref branch October 12, 2025 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants