Skip to content

[Sim] Add cascade erase for print/proc.print format/get_file producer chains#10204

Open
nanjo712 wants to merge 1 commit into
llvm:mainfrom
nanjo712:feat/sim-cleanup-fmt-chain
Open

[Sim] Add cascade erase for print/proc.print format/get_file producer chains#10204
nanjo712 wants to merge 1 commit into
llvm:mainfrom
nanjo712:feat/sim-cleanup-fmt-chain

Conversation

@nanjo712
Copy link
Copy Markdown
Contributor

This PR introduces a utility to cascade-delete sim.print and sim.proc.print along with their dependent sim.fmt (and sim.get_file) DAG nodes when they become dead.

This is motivated by lowering sim.print/sim.proc.print without relying on RegionDCE. Since RegionDCE is region-local, but the format dependency chain of a print may cross region boundaries (e.g., through if regions), using it would require walking up to a top-level procedural region (such as sv.always or hw.triggered) to perform cleanup.

I think it will be helpful to #10172.

void cascadeErasePrint(PrintFormattedOp op, mlir::RewriterBase &rewriter);
void cascadeErasePrint(PrintFormattedProcOp op, mlir::RewriterBase &rewriter);

/// TODO: Add explicit cycle detection helper if callers need local validation.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think we need a helper to detect cycle. But it's not quite related to the topic of this PR. So I think I would implement it in a follow-up PR.

Comment on lines +25 to +30
static bool isDeleteCascadable(Operation *op) {
return isa<FormatLiteralOp, FormatHexOp, FormatOctOp, FormatBinOp,
FormatScientificOp, FormatFloatOp, FormatGeneralOp, FormatDecOp,
FormatCharOp, FormatHierPathOp, FormatStringConcatOp, GetFileOp>(
op);
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It might be more appropriate to check this using a trait, as it would likely offer better maintainability. I'm wondering whether to add a trait to explicitly express this, or simply use Pure.

@fzi-hielscher
Copy link
Copy Markdown
Contributor

Could you elaborate why you think applying RegionDCE to the parent operation is a problem? After ProceduralizeSim we are left with hw.triggered operations that consist almost entirely of ops that should be dead after your converision to sv.write. So, to me RegionDCE seems like the perfect utility here.

The cleanup process in ProceduralizeSim on the other hand is something I'm not super happy about, despite writing it myself. 😅 It is a simple convergence loop which could have $O(n^2)$ time complexity, where $n$ is the number of sim.fmt.* operations. I have considered replacing it with RegionDCE, but convinced myself not to, because:

  • It would run over all operation within the hw.module of which usually only a small fraction will be sim.fmt.* ops.
  • After canonicalization all sim.fmt.concat operations should be flattened, which effectively puts a constant limit on the longest path in the format operation DAG. So, in practice the runtime should be in $O(n)$.

@nanjo712
Copy link
Copy Markdown
Contributor Author

Could you elaborate why you think applying RegionDCE to the parent operation is a problem?

My hesitation with RegionDCE is mostly about granularity.

The dead ops here are really the formatting / file-helper producer chain rooted at the removed sim.proc.print, whereas RegionDCE needs us to pick an enclosing region first. In practice that means walking up to some procedural root that is guaranteed to contain the whole subgraph, which felt less direct than just deleting the dead producer chain explicitly.

I also liked that the explicit cleanup keeps the transform narrowly scoped: it only removes the formatting / sim.get_file ops invalidated by this lowering, instead of potentially folding in unrelated dead ops in the same enclosing region.

So it is not that I think RegionDCE would be wrong here; I just found the explicit cleanup to be a better fit for the specific subgraph we are invalidating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants