circt icon indicating copy to clipboard operation
circt copied to clipboard

[ExportVerilog] Should ignore unknown top-level ops

Open teqdruid opened this issue 1 year ago • 4 comments

If we want to allow passes to run after ExportVerilog (e.g. to analyze the actual names assigned in ExportVerilog), ExportVerilog needs to ignore unknown ops instead of producing an error. (So that passes before ExportVerilog can leave behind ops which have information which needs to be combined with SV names.)

At least ops at the top level.

Is this the way we want to go with this? The other option would be to move all of the ExportVerilog side effects into PrepareForEmission. That includes the final SV names since passes may have to know about name mangling. Seems far simpler to me to run things after ExportVerilog.

teqdruid avatar Nov 17 '23 22:11 teqdruid

@darthscsi @seldridge I know we discussed this recently, but I don't recall the outcome of that discussion.

teqdruid avatar Nov 17 '23 22:11 teqdruid

I think this makes sense---have ExportVerilog ignore ops outside HW modules that it doesn't understand. We're already kind of in this situation with OM dialect, though ExportVerilog is aware of those ops.

seldridge avatar Nov 19 '23 00:11 seldridge

How about an opt-in approach to ExportVerilog based on an OpInterface? i.e. VerilogEmittableOpInterface. Could probably also move some of the naming/prep logic from PrepareForEmission over to that op, since there's a bunch of op-specific pattern matching going on in there (e.g. isExpressionAlwaysInline seems like something that can be attached to an interface).

I'd prefer opt-in as opposed to opt-out for the sake of not having every single operation in CIRCT explicitly say whether they care about a single (although at the moment obviously central to the entire flow) pass; that is. If we in theory had multiple emission paths, I wouldn't want to explicitly enumerate which paths MyDialect.MyOp opts out of.

mortbopet avatar Nov 29 '23 09:11 mortbopet

That's a really good idea. Why didn't I think of it?

I'd prefer opt-in as opposed to opt-out

Agreed

teqdruid avatar Nov 29 '23 21:11 teqdruid