[arcilator] Add JIT runtime environment library and stdio hooks by fzi-hielscher · Pull Request #7445 · llvm/circt

fzi-hielscher · 2024-08-06T00:55:58Z

This PR adds a dynamically linked runtime environment library to arcilator, which makes functions declared in the arcilator-runtime.h available for JIT compiled runs.

The runtime is intended to provide support functions that can be called directly from the generated IR, similar to the CRunnerUtils in LLVM. I went for a dynamic instead of a static library because I think there is some (limited) use in having the option to swap the runtime environment without having to recompile arcilator itself.

Notably, it provides a well defined interface for certain methods of the C standard library (currently fprintf, fputs, fputc). While it is technically possible to invoke them directly from the IR, going through the standard headers and a C preprocessor should avoid trouble caused by different libc implementations (glibc, musl, msvcrt, ucrt, ...).

PS: Just noticed that I switched from camel case to snake case. _arcLibcFprintf doesn't look right to me...

uenoku · 2024-08-06T06:23:52Z

                         << info.message() << "\n";
          });
+
+      arc_jit_runtime_env_deinit();


nit: you can use llvm::make_scope_exit to automatically call deinit.

Nice. Thanks!

uenoku · 2024-08-06T08:22:01Z

+// ---  Exports to the IR ---
+
+#ifdef _WIN32
+#define ARCEXPORT(rtype) extern "C" __declspec(dllexport) rtype __cdecl


Can you make this consistent with MLIR_RUNNERUTILS_EXPORT macro?

MLIR_RUNNERUTILS_EXPORT does not include extern "C" yet it looks like it is prefixed with extern "C" every time it is used. That seems odd to me.
The __cdecl is probably not required. It don't have access to my Windows PC right now, I'll check later today. Basically, I've just used every magic incantation here that I could come up with to increase the likelihood of it linking. 😅

maerhart · 2024-08-06T08:08:09Z

+    %ascii_em = llvm.mlir.constant(33 : i32) : i32
+    %ascii_lf = llvm.mlir.constant(10 : i32) : i32
+    %ascii_l = llvm.mlir.constant(108 : i32) : i32
+    %in_clk = arc.root_input "clk", %arg0 {offset = 0 : i32} : (!arc.storage<1>) -> !arc.state<i1>


This seems to be unused. Was there some issue with the storage lowering when removing this?

No, tbh I didn't even try without at least one port as I was unsure whether this is a supported use-case.
Looks like it is working just fine. 👍

maerhart · 2024-08-06T08:33:44Z

+ARC_ENV_DECL_GET_PRINT_STREAM(id) {
+  (void)id;
+  return stderr;
+}


I'm wondering if it would make sense to let the id influence whether stderr or stdout is returned?

Yeah, I've added this in anticipation of #7111. The problem is, AFAIK we don't have any lowering path that would support this, let alone an agreed on concept for file descriptors / output streams in the IR. I had a short exchange with @seldridge on this in the discord channel some weeks ago. For now we could at best add a command line flag to control this globally.

Ah I see. I was just thinking about adding an if-statement here to allow the LLVM call operation to choose between stderr and stdout when writing MLIR integration tests and examples by hand. But this is not important at all. We can also just leave it as is until we have a proper story for file descriptors.

maerhart · 2024-08-06T08:34:52Z

+#ifdef _WIN32
+#define ARCEXPORT(rtype) extern "C" __declspec(dllexport) rtype __cdecl


I've no clue about those windows specific things, but thanks for making this work with Windows as well! 🚀

maerhart · 2024-08-06T08:44:54Z

@@ -0,0 +1,23 @@
+// Arcilator JIT runtime environment API facing the arcilator.cpp


We should probably also add the standard header comments here.

Yes, nice catch. I'm a little surprised the CI did not complain about it. The arcilator-runtime.h and arcilator-header-cpp.py should probably also at least get a license identifier, shouldn't they?

Yeah, seems like it slipped through there.

maerhart · 2024-08-06T08:45:09Z

@@ -0,0 +1,7 @@
+#include "../arcilator-runtime.h"


We should probably add the standard header comments here.

maerhart

LGTM! Thanks!

fabianschuiki

LGTM! Thanks for adding this 🥳

Can't wait for us to finally get the VCD writing out of the runtime header and built into the model a bit more thoroughly. That will make the JIT pretty much equivalent to a separately-compiled model 😄

fzi-hielscher · 2024-08-06T18:06:30Z

That does make me wonder how to proceed from here. When writing this PR's code I didn't want to mess up existing build flows. So, the "arcilator-runtime" remains a header library and we simply provide a compiled binary of it to the JIT runner. That comes at the cost of mixing declarations and definitions in the header.

If our goal is to move stuff out of the header, maybe it would be better if we had an arcilator-runtime.cpp to put static (i.e., model independent) logic in? The compiled library would then be usable for both JIT and conventional flows. I'm specifically thinking of the parsing logic for .mem files. A header file is not a great place to put that. But I think dynamically building it into the MLIR model makes even less sense.

What do you think?

fabianschuiki · 2024-08-06T20:55:37Z

Hmmm yeah I think it would make sense to actually compile a runtime library, link against it from the JIT, and also install it as a standalone library alongside the rest of CIRCT. If you use arcilator to build an object file, you can link against that and optionally the runtime in whatever build flow you're using. And if you use arcilator in JIT mode, you also have the library available.

fzi-hielscher · 2024-08-07T14:05:47Z

I've brought the export macro closer to MLIR_RUNNERUTILS_EXPORT as @uenoku suggested. On my local runs the test produces the expected output for Windows, MacOS, and Linux (glibc). So, I'm about 50% sure this will not break in the nightly tests. 😬

uenoku · 2024-08-07T17:18:12Z

Thank you @fzi-hielscher!

…ks (llvm#7445)" This reverts commit 30ed753.

fzi-hielscher added 5 commits August 6, 2024 01:37

Add arcilator JIT runtime library.

bde442f

Add test

704cd6e

fixup! Add arcilator JIT runtime library.

139700b

CMake Tweaks

bde3a08

fixup! Add arcilator JIT runtime library.

c8ea000

fzi-hielscher marked this pull request as ready for review August 6, 2024 01:14

fzi-hielscher requested review from fabianschuiki and maerhart as code owners August 6, 2024 01:14

uenoku reviewed Aug 6, 2024

View reviewed changes

maerhart reviewed Aug 6, 2024

View reviewed changes

fzi-hielscher added 3 commits August 6, 2024 13:07

Tweak init/deinit.

2a8d7f7

Added header comments.

0e413e0

Remove unused clock from test.

7230394

maerhart approved these changes Aug 6, 2024

View reviewed changes

fabianschuiki approved these changes Aug 6, 2024

View reviewed changes

fzi-hielscher added 3 commits August 7, 2024 12:57

Tweak export macros

330e445

Add attribute to ARCJITENV_API for consistency.

d5db8f6

Merge branch 'main' into arc-jit-env

faa0b2b

fzi-hielscher merged commit 30ed753 into llvm:main Aug 7, 2024

fzi-hielscher mentioned this pull request Aug 28, 2024

[Arc] Implement memory initializers #7559

Draft

fzi-hielscher added a commit to fzi-hielscher/circt that referenced this pull request May 15, 2025

Revert "[arcilator] Add JIT runtime environment library and stdio hoo…

9165872

…ks (llvm#7445)" This reverts commit 30ed753.

		#ifdef _WIN32
		#define ARCEXPORT(rtype) extern "C" __declspec(dllexport) rtype __cdecl

		@@ -0,0 +1,23 @@
		// Arcilator JIT runtime environment API facing the arcilator.cpp

Conversation

fzi-hielscher commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fzi-hielscher Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maerhart left a comment

Choose a reason for hiding this comment

Uh oh!

fabianschuiki left a comment

Choose a reason for hiding this comment

Uh oh!

fzi-hielscher commented Aug 6, 2024

Uh oh!

fabianschuiki commented Aug 6, 2024

Uh oh!

fzi-hielscher commented Aug 7, 2024

Uh oh!

uenoku commented Aug 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fzi-hielscher commented Aug 6, 2024 •

edited

Loading

fzi-hielscher Aug 6, 2024 •

edited

Loading