Skip to content

[Arc][Conversion] Lower sim.clocked_terminate to exit in LowerArcToLLVM#10089

Open
nanjo712 wants to merge 2 commits intollvm:mainfrom
nanjo712:feat/arc-sim-terminate-lowering
Open

[Arc][Conversion] Lower sim.clocked_terminate to exit in LowerArcToLLVM#10089
nanjo712 wants to merge 2 commits intollvm:mainfrom
nanjo712:feat/arc-sim-terminate-lowering

Conversation

@nanjo712
Copy link
Copy Markdown
Contributor

@nanjo712 nanjo712 commented Mar 31, 2026

Description

This PR addresses the issue that ArcToLLVM Pass cannot correctly downgrade from sim.clocked_terminate to LLVM IR. The current implementation strategy adds 8 bytes to the reserved portion of the Model State header (8 bytes were already reserved for simulation timing; the additional 8 bytes are for alignment), with the first byte used as the terminateFlag.

Technical Changes

Reserved offset 8 in the simulation state for a termination flag by increasing kStateOffset to 16 in AllocateState.cpp.

Updated ArcToLLVM to write status codes (1 for success, 2 for failure) to this flag.

Testing

Added a new regression test: test/arcilator/sim_clocked_terminate.mlir.

Updated sim_clocked_terminate.mlir and allocate_state.mlir to verify the new memory layout and store logic.

Reference

Fixes #10087.

Assisted-by: Gemini 3 Flash (Used for English translation)

@nanjo712 nanjo712 force-pushed the feat/arc-sim-terminate-lowering branch from 106a193 to cca53ee Compare March 31, 2026 12:56
@fzi-hielscher
Copy link
Copy Markdown
Contributor

Thank you for your contribution, @nanjo712. It would be great to finally have support for the terminate operation in arcilator. However, I'm skeptical if simply calling exit is the right approach here. We need to differentiate between terminating the simulated instance vs. terminating the entire simulation process. There is a bunch of code that we may have to execute after something like $finish is called:

  • Anything contained in an arc.final op
  • The clean-up routine of the ArcRuntime library
  • Code that is part of the simulation driver, e.g., arcilator itself in case of JIT executions

One option to solve this would be to add a return value to the eval function that indicates whether the simulation should continue. Alternatively, we could add a dedicated flag to the simulation state (similar to how the simulation time is currently stored), which gets set by the terminate operation and has to be checked before or after each eval call.

Please also consider our policy on AI assisted contributions: https://llvm.org/docs/AIToolPolicy.html

@fzi-hielscher fzi-hielscher added the Arc Involving the `arc` dialect label Mar 31, 2026
@nanjo712 nanjo712 force-pushed the feat/arc-sim-terminate-lowering branch from 7c7ade4 to 86ceb84 Compare April 1, 2026 04:34
@nanjo712
Copy link
Copy Markdown
Contributor Author

nanjo712 commented Apr 1, 2026

@fzi-hielscher
I have updated the PR to use a dedicated flag in the simulation state (the second option you suggested).

Regarding the LLVM AI Tool Policy: I used AI tools solely for documentation translation and to assist in navigating and understanding the existing CIRCT codebase. All code, logic designs, and test cases included in this PR were manually authored and verified by me.

@nanjo712 nanjo712 force-pushed the feat/arc-sim-terminate-lowering branch from 86ceb84 to 5eb102f Compare April 1, 2026 04:58
@uenoku
Copy link
Copy Markdown
Member

uenoku commented Apr 1, 2026

To echo @fzi-hielscher, AI policy applies to PR descriptions as well. Please avoid directly pasting verbose LLM output into PR description since it's unnecessary verbose and hard to understand the real intent of the PR. (My apologies if the current description was written manually!) SI Even if the code was developed entirely without AI, verbose PR description can give the impression that the entire PR was created by LLM, which ultimately diminishes your hard work.

This is from LLVM AI policy:

To ensure sufficient self review and understanding of the work, it is strongly recommended that contributors write PR descriptions themselves (if needed, using tools for translation or copy-editing). The description should explain the motivation, implementation approach, expected impact, and any open questions or uncertainties to the same extent as a contribution made without tool assistance.

@nanjo712
Copy link
Copy Markdown
Contributor Author

nanjo712 commented Apr 1, 2026

@uenoku
Thank you for the pointer and the reminder! I sincerely apologize for the verbose description.
As a non-native speaker, I sometimes rely on AI tools like Gemini 3 Flash to refine my English phrasing and ensure the description meets the community's standards for formality and clarity. I will rewrite the PR description to be more concise.

@uenoku
Copy link
Copy Markdown
Member

uenoku commented Apr 1, 2026

No need to apologize! Thank you for sending a PR 😄 It's totally fine to use LLM or other tools for translation.

- Implement lowering for `sim.clocked_terminate` by writing a status code
  (1 for success, 2 for failure) to the state at offset 8.
- Increase `kStateOffset` to 16 in `AllocateState.cpp` to reserve space
  for the termination flag.
- Use a branch to the exit block instead of calling `exit()` to allow
  the simulation instance to terminate without killing the host process.
- Add test cases in `sim_clocked_terminate.mlir`.
@nanjo712 nanjo712 force-pushed the feat/arc-sim-terminate-lowering branch from 5eb102f to 9e70177 Compare April 1, 2026 05:30
@nanjo712
Copy link
Copy Markdown
Contributor Author

nanjo712 commented Apr 1, 2026

Hi, I noticed that the CI job "Build and Test / Build and Test (clang, clang++, Debug, ON, ON) (pull_request)" is consistently failed with 'The operation was canceled.'

I've checked the logs, and there are no explicit build errors; the process simply stops. Could you help check if this is due to a specific environment timeout, a resource limit?

Copy link
Copy Markdown
Contributor

@fzi-hielscher fzi-hielscher left a comment

Choose a reason for hiding this comment

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

No worries. It was indeed the PR description which made me assume AI assistance. As @uenoku said, there is nothing inherently wrong about using it, we just prefer it to be disclosed. Thanks for adding the information.

The CI is unfortunately broken at the moment and we get randomly canceled runs on all PRs. Sorry for the confusion, I don't think there is anything wrong with your code.

@nanjo712
Copy link
Copy Markdown
Contributor Author

nanjo712 commented Apr 1, 2026

I have tried to implement it according to the suggestion. Please take a look when you have time. Thank you.

@nanjo712 nanjo712 requested a review from fzi-hielscher April 2, 2026 04:06
Copy link
Copy Markdown
Contributor

@fzi-hielscher fzi-hielscher left a comment

Choose a reason for hiding this comment

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

Thanks! This is already looking pretty good. A few more suggestions:

@nanjo712 nanjo712 force-pushed the feat/arc-sim-terminate-lowering branch from 585f8e3 to fbdfb73 Compare April 2, 2026 12:31
@nanjo712 nanjo712 requested a review from fzi-hielscher April 2, 2026 12:46
Copy link
Copy Markdown
Contributor

@fzi-hielscher fzi-hielscher left a comment

Choose a reason for hiding this comment

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

One last nit, otherwise LGTM. 👍

@nanjo712 nanjo712 force-pushed the feat/arc-sim-terminate-lowering branch from fbdfb73 to 5f4ea01 Compare April 3, 2026 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Arc Involving the `arc` dialect

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[arcilator] Failed to legalize 'sim.clocked_terminate' and dangling macro references

3 participants