Create a minimal C API #196

GeorgeLyon · 2020-11-02T21:12:28Z

No description provided.

darthscsi

Please add a test case which uses the library. See llvm/test/CAPI

darthscsi · 2020-11-02T23:31:58Z

CMakeLists.txt

+  PATTERN "*.def"
+  PATTERN "*.h"
+  PATTERN "*.inc"
+  PATTERN "*.td"


There are no inc, td, or def files in the c-api.

I copied this verbatim from LLVM. I don't see a problem leaving these in unless we explicitly want to take the stance that we are trying to build this API without these file types.

lattner

Thank you for tackling this, LGTM with the @darthscsi 's comment addressed. Thanks!

lattner · 2020-11-03T06:08:52Z

include/circt-c/RTLDialect.h

@@ -0,0 +1,30 @@
+/*===-- circt-c/RTLDialect.h - C API for RTL dialect --------------*- C -*-===*\
+|*                                                                            *|
+*|


nit: please fill in the file header

lattner · 2020-11-03T06:11:00Z

include/circt-c/RTLDialect.h

+extern "C" {
+#endif
+
+/** Registers the RTL dialect with the given context. This allows the


Ok, I see the MLIR C bindings are doing the same thing, but this still seems a bit weird to me: The use of /**/ comments is very inclusive to old C compilers, but I think that BCPL style // comments were part of ansi c 89/90, so I think they are ok to use. Doing so gets us better alignment with the c++ side of things. Could you ask the MLIR forum what the rationale here is, and whether using // would be ok with them?

https://llvm.discourse.group/t/a-quick-question-about-comment-styles/2144

Seems like there is a policy, and I'm not planning on rocking the boat too much yet :)

lattner · 2020-11-03T06:11:37Z

lib/CAPI/RTL/RTLDialect.cpp

@@ -0,0 +1,21 @@
+//===- RTLDialect.cpp - C Interface for RTL Dialect -----------------------===//
+//


same nit as the header.

fabianschuiki · 2020-11-03T08:23:54Z

This is great! I think I will do a similar thing for LLHD, in order to be able to switch Moore from the Rust LLHD library to CIRCT as a foreign library at some point.

GeorgeLyon · 2020-11-03T18:12:36Z

I'm a little confused at how the tests work.
In MLIR, CMakeLists explicitly includes the test subdirectory: https://github.com/llvm/llvm-project/blob/00eff96e1d02969ff1c1416a266b4d29506cd1c0/mlir/test/CMakeLists.txt#L1
In Circt, on the other hand, doesn't explicitly add subdirectories and seems like it is discovering tests automatically... https://github.com/llvm/circt/blob/master/test/CMakeLists.txt

Can someone please clarify what the best path forward is?

abrachet · 2020-11-03T21:03:02Z

lib/CAPI/RTL/RTLDialect.cpp

+#include "mlir/CAPI/IR.h"
+#include "mlir/CAPI/Support.h"
+
+void mlirContextRegisterRTLDialect(MlirContext context) {


Am I missing something or should these symbols be extern "C"?

I mirrored the structure of StandardDialect.cpp: https://github.com/llvm/llvm-project/blob/64c0c9f01511dc300b29e7a20a13958c5932e314/mlir/lib/CAPI/Standard/StandardDialect.cpp
They are declared as extern "C" in the header:

circt/include/circt-c/RTLDialect.h

Line 17 in 4b01c0a

extern "C" {

lattner · 2020-11-04T00:28:53Z

Cool, I think this looks good with a few nits about the file header. I agree that you should keep the comment style consistent with the MLIR style. If/when they change, we can change to match.

darthscsi · 2020-11-04T16:29:17Z

I'm a little confused at how the tests work.
In MLIR, CMakeLists explicitly includes the test subdirectory: https://github.com/llvm/llvm-project/blob/00eff96e1d02969ff1c1416a266b4d29506cd1c0/mlir/test/CMakeLists.txt#L1
In Circt, on the other hand, doesn't explicitly add subdirectories and seems like it is discovering tests automatically... https://github.com/llvm/circt/blob/master/test/CMakeLists.txt

Can someone please clarify what the best path forward is?

add_subdirectory is a directive to cmake. The reason it is doing it is to pickup the cmake-based build of the executable in the test directory. add_subdirectory isn't directing the testing framework to do anything, it's just building the c-based test executable and registering it so the test target can depend on it.

GeorgeLyon · 2020-11-04T17:19:23Z

I'm a little confused at how the tests work.
In MLIR, CMakeLists explicitly includes the test subdirectory: https://github.com/llvm/llvm-project/blob/00eff96e1d02969ff1c1416a266b4d29506cd1c0/mlir/test/CMakeLists.txt#L1
In Circt, on the other hand, doesn't explicitly add subdirectories and seems like it is discovering tests automatically... https://github.com/llvm/circt/blob/master/test/CMakeLists.txt

Can someone please clarify what the best path forward is?

It looks like ".mlir" style tests are handled differently from C tests which need to be compiled as part of the cmake project in order to be run. As such, C tests need to use "add_subdirectory" to provide their own CMakeLists.txt, which "mlir" and "fir" tests do not. The reason we don't have this yet in circt is because there are no C tests yet.

GeorgeLyon · 2020-11-04T18:07:44Z

So in my latest attempt, I'm getting the following error: https://github.com/llvm/circt/runs/1354327993#step:8:363

I've tried to copy what is happening in the MLIR C test, but for some reason it isn't finding the executable (which is present after the build and produces the correct output).

╭ ~/Developer/Projects/circt on master                                                              
╰─○ ./build/bin/circt-capi-ir-test    
@registration
0

Does anyone have any thoughts as to what I may be missing? @darthscsi

GeorgeLyon · 2020-11-04T18:58:48Z

This was the spot I missed 181011e

lattner · 2020-11-04T22:05:31Z

Cool, thank you George. It looks like MLIR just switched their comment style, could you adapt this code to follow the new approach in a follow-on? Thanks,

-Chris

GeorgeLyon · 2020-11-04T22:34:59Z

Race conditions always get you :(

lattner · 2020-11-05T00:58:22Z

If only there were some way to define them away!

George added 6 commits October 29, 2020 10:30

Enable cloning llvm submodule over HTTP

483fef3

Introduce C API

963d8b4

Merge branch 'master' of github:llvm/circt

063c1bd

Undo unrelated changes

7c3e8e6

clang-format

6b935fa

More format

c6230f4

GeorgeLyon marked this pull request as ready for review November 2, 2020 21:50

darthscsi reviewed Nov 2, 2020

View reviewed changes

lattner approved these changes Nov 3, 2020

View reviewed changes

George added 3 commits November 3, 2020 11:45

Merge branch 'master' of github:llvm/circt

da489c5

Add Header Comments

e5aa114

Format

4b01c0a

abrachet reviewed Nov 3, 2020

View reviewed changes

Add basic test

79e607f

Add missing incantation

181011e

Format

ee9be70

GeorgeLyon merged commit 2a9eb35 into llvm:master Nov 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a minimal C API #196

Create a minimal C API #196

GeorgeLyon commented Nov 2, 2020

darthscsi left a comment

darthscsi Nov 2, 2020

GeorgeLyon Nov 3, 2020

lattner left a comment

lattner Nov 3, 2020

lattner Nov 3, 2020

GeorgeLyon Nov 3, 2020

lattner Nov 3, 2020

fabianschuiki commented Nov 3, 2020

GeorgeLyon commented Nov 3, 2020

abrachet Nov 3, 2020

GeorgeLyon Nov 3, 2020

lattner commented Nov 4, 2020

darthscsi commented Nov 4, 2020

GeorgeLyon commented Nov 4, 2020

GeorgeLyon commented Nov 4, 2020

GeorgeLyon commented Nov 4, 2020

lattner commented Nov 4, 2020

GeorgeLyon commented Nov 4, 2020

lattner commented Nov 5, 2020

		@@ -0,0 +1,21 @@
		//===- RTLDialect.cpp - C Interface for RTL Dialect -----------------------===//
		//

Create a minimal C API #196

Create a minimal C API #196

Conversation

GeorgeLyon commented Nov 2, 2020

darthscsi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lattner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianschuiki commented Nov 3, 2020

GeorgeLyon commented Nov 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lattner commented Nov 4, 2020

darthscsi commented Nov 4, 2020

GeorgeLyon commented Nov 4, 2020

GeorgeLyon commented Nov 4, 2020

GeorgeLyon commented Nov 4, 2020

lattner commented Nov 4, 2020

GeorgeLyon commented Nov 4, 2020

lattner commented Nov 5, 2020