Thanks for updating the PR @jpienaar.

Thanks for your review @jpienaar!

Could you also add tests to test/Dialect/EmitC/ops.mlir and maybe also to test/Dialect/EmitC/invalid_ops.mlir?

Updating D149963: [mlir][emitc] Add add and sub operations

Updating D149963: [mlir][emitc] Add add and sub operations

Updating D149963: [mlir][emitc] Add add and sub operations

Updating D149963: [mlir][emitc] Add add and sub operations

I added myself and especially @simon-camp as reviewer, since I might be oof when this is ready for review.

Thanks!

In D147992#4301154, @QuqqU wrote:

In D147992#4299476, @marbre wrote:

Thanks for the patch! Tested locally, LGTM. Unfortunately, I have some problems with arcanist grabbing the complete patch. I will try to land it tomorrow.

I greatly appreciate for your help :>
If there is an issue(eg. metadata) on my patch, please let me know.

Thanks for the patch! Tested locally, LGTM. Unfortunately, I have some problems with arcanist grabbing the complete patch. I will try to land it tomorrow.

In D147992#4299008, @QuqqU wrote:

@marbre @simon-camp Could you mind if I ask you push it on my behalf?
I have no commit access because this is the first time.
I really appreciate!

Currently no reviewers are specified. Would you like @simon-camp and me to review? We initially pushed the code and mostly maintain it.

I added @simon-camp and myself as reviewers, as we initial pushed the code and mostly maintain it. We will take a look early next week.

In D147992#4262163, @QuqqU wrote:

ping

Please take a look, and let me know if there is an issue :)

As far as I see this should be fine, even though I am not 100% familiar with the exact mechanism implemented here.

This was presented as part of my talk How to Build your own MLIR Dialect at FOSDEM 2023.

Thanks for driving this! We actually had something similar on our long term roadmap and think something function meta attribute like would be desirable. However, for the omp dialect there it is currently discussed on discourse if to go for an omp.function instead of a dialect attribute, see https://discourse.llvm.org/t/rfc-omp-module-and-omp-function-vs-dialect-attributes-to-encode-openmp-properties/67998. I think the same discussion applies here as well. Instead of having emitc.func_attr together with func.func we might want to have emitc.func?

This was presented as part of my talk How to Build your own MLIR Dialect at FOSDEM 2023.

Thanks for adding me @jpienaar and thanks @christopherbate for improving the EmitC dialect! LGTM.

In D130350#3684336, @mstorsjo wrote:

In D130350#3684169, @powderluv wrote:

I tried the patch locally to build torch-mlir https://github.com/llvm/torch-mlir/issues/1094 and it still fails cross-compile.

If you apply the to torch-mlir, you also need a corresponding change to the one in flang/CMakeLists.txt in this patch, where set(MLIR_TABLEGEN_EXE $<TARGET_FILE:mlir-tblgen>) is removed - I presume that torch-mlir has something similar.

I am currently out-of-office and don‘t have any access to my build machines. Therefore, I cannot test and confirm that the patch doesn‘t have unintended side effects. However, there is definitely a need to add a dependency to the tablgen generated header files. Sorry for missing this as part of https://reviews.llvm.org/D124851. I would suggest to take a look how this is done for MLIR dialects, where explicits deps to the generated targets are passed to add_mlir_dialect_library like in https://github.com/llvm/llvm-project/blob/04b419048955fc33718ba97e79f3558b6a27830e/mlir/lib/Dialect/EmitC/IR/CMakeLists.txt#L7-L9. It might not be necessary to touch AddLLVM and I assume the deps should rather be on the *IncGen target,

The missing dep on MLIRIR actually showed up as a race condition in the build process on a GH runner.

Thanks for the review @jpienaar! I will address your comments before landing.

Updating D124002: [mlir][emitc] Disallow !emitc.opaque pointers

Please excuse if my previous reply was confusing. I checked more carefully and think these changes are good to go.

Thanks for working on this Stella! Untangling the functions/marcos is a bit hard. Thus, I am mainly commenting to make sure I fully understand the patch :)

In D120098#3339041, @jpienaar wrote:

In D120098#3331073, @marbre wrote:

As an example, the canonicalization pass would transform

func @myfunc() {
  %0 = "emitc.constant"() {value = 0 : i32} : () -> i32
  %1 = "emitc.constant"() {value = 0 : i32} : () -> i32
  %2 = emitc.apply "&"(%0) : (i32) -> !emitc.ptr<i32>
  %3 = emitc.apply "&"(%1) : (i32) -> !emitc.ptr<i32>
  emitc.call "write"(%2, %3) : (!emitc.ptr<i32>, !emitc.ptr<i32>) -> ()
  return
}

into

func @myfunc() {
  %0 = "emitc.constant"() {value = 0 : i32} : () -> i32
  %1 = emitc.apply "&"(%0) : (i32) -> !emitc.ptr<i32>
  %2 = emitc.apply "&"(%0) : (i32) -> !emitc.ptr<i32>
  emitc.call "write"(%1, %2) : (!emitc.ptr<i32>, !emitc.ptr<i32>) -> ()
  return
}

resulting in pointer aliasing. Here, the variable op would be helpful.

Could you elevate this to be part of the commit description and expand on the pains? (I think you'd want to expand in the op descriptions too what variety to use and why).

Address review comments

As an example, the canonicalization pass would transform

func @myfunc() {
  %0 = "emitc.constant"() {value = 0 : i32} : () -> i32
  %1 = "emitc.constant"() {value = 0 : i32} : () -> i32
  %2 = emitc.apply "&"(%0) : (i32) -> !emitc.ptr<i32>
  %3 = emitc.apply "&"(%1) : (i32) -> !emitc.ptr<i32>
  emitc.call "write"(%2, %3) : (!emitc.ptr<i32>, !emitc.ptr<i32>) -> ()
  return
}

into

func @myfunc() {
  %0 = "emitc.constant"() {value = 0 : i32} : () -> i32
  %1 = emitc.apply "&"(%0) : (i32) -> !emitc.ptr<i32>
  %2 = emitc.apply "&"(%0) : (i32) -> !emitc.ptr<i32>
  emitc.call "write"(%1, %2) : (!emitc.ptr<i32>, !emitc.ptr<i32>) -> ()
  return
}

resulting in pointer aliasing. Here, the variable op would be helpful.