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.

Updating D119337: [mlir][emitc] Add a pointer type

@jpienaar I adressed your comments with the latest revision.

Updating D119337: [mlir][emitc] Add a pointer type

Updating D118154: [mlir] Don't emit unused labels

I recently noticed that arith::ConstantOp support was added to the Cpp emitter with this commit. @jpienaar and @mehdi_amini, do we really want to add support for such ops to the printer? The alternative would be to add conversions, like the arith.constant to emitc.constant one which I have implemented in our mlir-emitc repo: https://github.com/iml130/mlir-emitc/blob/c3b7bf093417ecd50e4b246ffd85079dbd75e359/lib/Dialect/EmitC/Conversion/ArithToEmitC.cpp#L27-L38.

LGTM. Thanks for improving the EmitC!

Thanks for taking over!

In D104632#2972448, @mehdi_amini wrote:

I'll leave approval to @jpienaar, the current version is fine with me, thanks!

Refactor translation cmdline arguments

Thanks for the review @jpienaar! I have updated our patch accordingly.

Address review comments

With https://github.com/llvm/mlir-www/pull/83 merged (allowing to include custom docs in auto-generated ones) and me having finally had some time to write a first markdown documentation (we can still iterate and improve that one!) I would kindly ask @rriddle, @mehdi_amini and @jpienaar to re-review the patch :)

Add markdown documentation and dialect description

In D106541#2897051, @rriddle wrote:

Have you looked into fixing the issues with gen-dialect-doc? Or is it not easily feasible?

In D106541#2896241, @jpienaar wrote:

OOC when would one want to just have the type constraints? I was thinking most were moving towards full doc generation

Updating D104632: [mlir] Add Cpp emitter

Thanks for all review comments. Most comments should be addressed with the next revision. As discussed, the emitter's API is now significantly closed (only translateToCpp is accessible). The revision also drops the emitter's restriction to only generate C code. This is of course still possible and depends (as before) on the implemented conversions, but isn't checked in the emitter itself anymore. What the revision lacks is a dedicated markdown documentation. Suggestion to where to place such docs in the docs/ folder are welcome. So far the other targets don't have separate docs or should those live side-by-side to the EmitC dialect?