fix operand names conflicting with local variables in the generated functions #74

jumerckx · 2024-06-02T21:19:14Z

I added all local variables used in the generated functions to the reservedKeyword list. In due time a more robust solution could be implemented but this fixes quite a lot of cases already.

Should PRs like these include the generated code as well? I've added it as separate commits.
I didn't change the API bindings script but the bindings have changed nevertheless, has it been a while since this has been updated or are the results machine-dependent?

The dialect binding generation threw an error for LLVM14, I ran it using Julia 1.10.3, not sure what's up.

Included from /tmp/jl_TbvR47/x86_64-linux-gnu-libgfortran5-cxx11-libstdcxx30-llvm_version+14-julia_version+1.9.0/destdir/include/mlir/Dialect/OpenACC/OpenACCOps.td:16:
Included from /tmp/jl_TbvR47/x86_64-linux-gnu-libgfortran5-cxx11-libstdcxx30-llvm_version+14-julia_version+1.9.0/destdir/include/mlir/IR/EnumAttr.td:12:
/tmp/jl_TbvR47/x86_64-linux-gnu-libgfortran5-cxx11-libstdcxx30-llvm_version+14-julia_version+1.9.0/destdir/include/mlir/IR/OpBase.td:2130:1: error: Record `AttrSizedOperandSegments' does not have a field named `dependentTraits'!

def AttrSizedOperandSegments : NativeOpTrait<"AttrSizedOperandSegments">;
^
ERROR: LoadError: failed process: Process(`/home/jumerckx/.julia/scratchspaces/bfde9dd4-8f40-4a1e-be09-1475335e1c92/build/bin/mlir-jl-tblgen --generator=jl-op-defs --disable-module-wrap /tmp/jl_TbvR47/x86_64-linux-gnu-libgfortran5-cxx11-libstdcxx30-llvm_version+14-julia_version+1.9.0/destdir/include/mlir/Dialect/OpenACC/OpenACCOps.td -I /tmp/jl_TbvR47/x86_64-linux-gnu-libgfortran5-cxx11-libstdcxx30-llvm_version+14-julia_version+1.9.0/destdir/include/mlir/Dialect/OpenACC -I /tmp/jl_TbvR47/x86_64-linux-gnu-libgfortran5-cxx11-libstdcxx30-llvm_version+14-julia_version+1.9.0/destdir/include -o /tmp/jl_CT9KRy`, ProcessExited(1)) [1]

Stacktrace:
  [1] pipeline_error
    @ Base ./process.jl:565 [inlined]
  [2] run(::Cmd; wait::Bool)
    @ Base ./process.jl:480
  [3] run
    @ ./process.jl:477 [inlined]
  [4] (::var"#4#7"{String, String})(td::String)
    @ Main ~/.julia/dev/MLIR/bindings/make.jl:264
  [5] iterate
    @ ./generator.jl:47 [inlined]
  [6] _collect(c::Vector{String}, itr::Base.Generator{Vector{String}, var"#4#7"{String, String}}, ::Base.EltypeUnknown, isz::Base.HasShape{1})
    @ Base ./array.jl:854
  [7] collect_similar
    @ ./array.jl:763 [inlined]
  [8] map
    @ ./abstractarray.jl:3282 [inlined]
  [9] (::var"#3#6"{VersionNumber, VersionNumber})(prefix::Prefix)
    @ Main ~/.julia/dev/MLIR/bindings/make.jl:253
 [10] #66
    @ ~/.julia/packages/BinaryBuilderBase/JOwzH/src/Prefix.jl:46 [inlined]
 [11] mktempdir(fn::BinaryBuilderBase.var"#66#68"{var"#3#6"{VersionNumber, VersionNumber}}, parent::String; prefix::String)
    @ Base.Filesystem ./file.jl:766
 [12] mktempdir
    @ Base.Filesystem ./file.jl:762 [inlined]
 [13] temp_prefix(func::var"#3#6"{VersionNumber, VersionNumber})
    @ BinaryBuilderBase ~/.julia/packages/BinaryBuilderBase/JOwzH/src/Prefix.jl:42
 [14] top-level scope
    @ ~/.julia/dev/MLIR/bindings/make.jl:207
in expression starting at /home/jumerckx/.julia/dev/MLIR/bindings/make.jl:204

jumerckx · 2024-06-02T21:43:53Z

CI has me confused.

ERROR: LoadError: ArgumentError: invalid type for argument str in method definition for print_callback at /home/runner/work/MLIR.jl/MLIR.jl/src/IR/IR.jl:31

jumerckx · 2024-06-04T16:43:30Z

The reason for the error is that the bindings in this PR have type definitions included in the different libmlir_h.jl files.
How come these aren't included on main?
@mofeing, how did you generate the bindings before?

mofeing · 2024-06-05T11:14:47Z

Ah yeah, I had to remove them by hand. The reason is that types declared in v14, v15, ... are different types with the same names. And we can not do dynamic dispatch of types for defining new types or functions.

For example, check out the IR.AffineMap definition:
https://github.com/JuliaLabs/MLIR.jl/blob/b8ab8252df1b444a9f98822f19cb3f0bca96164c/src/IR/AffineMap.jl#L1-L8

That API.MlirAffineMap could refer to any of the versioned types! Actually, what would happen is that it would choose the one from the Julia version used when precompiling. And whenever you switch to another MLIR version (like when using Coil.jl or Reactant.jl), Julia would crash because the types don't match.

What I did in the end is manually move all the type definitions to API/Types.jl so they can be shared across all MLIR versions.
This is not really a problem because almost all types are opaque pointers to MLIR-backed implementation, and the ones that are not opaque pointers have seen no modification between versions.

I would love to have it done automatically by mlir_jl_tblgen, but I'm not sure how that would be implemented.

jumerckx · 2024-06-05T13:36:02Z

I would love to have it done automatically by mlir_jl_tblgen, but I'm not sure how that would be implemented.

IIUC, this shouldn't be handled in mlir-jl-tblgen but rather in the Clang.jl generator?
I think it should be possible to add the manually wrapped structs to an output_ignorelist or something in the Clang.jl config.
I'll check if I can get that going later.

mofeing · 2024-06-05T17:52:05Z

Awesome! Thanks @jumerckx

jumerckx · 2024-06-05T18:09:07Z

Hmm, the last hurdle is the dialect wrappers for MLIR/LLVM 14. These keep failing with an error like in the OP. Skipping the failing dialect just uncovers the same problem with the subsequent dialects.

mofeing · 2024-06-05T18:21:10Z

Weird. I'm sure what's happening. I will try to run it in my computer tomorrow.

jumerckx · 2024-06-05T18:26:06Z

I will try to run it in my computer tomorrow.

Cool, thanks!

I also shouldn't forget to run formatting before I merge 😅

mofeing · 2024-06-05T20:55:09Z

mmm i had no problem generating the bindings. could it be that your env is dirty?

jumerckx · 2024-06-05T21:20:16Z

mmm i had no problem generating the bindings. could it be that your env is dirty?

That's curious. I'm pretty sure my env is clean. I built mlir_jl_tblgen using the build_local.jl script and manually edited make.jl to use that version of mlir_jl_tblgen. This should work, right?

What version of Julia did you use? I used 1.10 but tried 1.7 as well.
With 1.7, building mlir_jl_tblgen even fails...

/home/jumerckx/.julia/dev/MLIR/deps/tblgen/jl-generators.cc:39:10: fatal error: mlir/TableGen/Class.h: No such file or directory
   39 | #include "mlir/TableGen/Class.h"
      |          ^~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
gmake[2]: *** [CMakeFiles/mlir-jl-tblgen.dir/build.make:90: CMakeFiles/mlir-jl-tblgen.dir/jl-generators.cc.o] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:272: CMakeFiles/mlir-jl-tblgen.dir/all] Error 2
gmake: *** [Makefile:136: all] Error 2
ERROR: LoadError: failed process: Process(`/home/jumerckx/.julia/artifacts/a8d27fab138de0785cb0eb3157737c9fce5d6793/bin/cmake --build /tmp/jl_qx2xh7 --target install`, ProcessExited(2)) [2]

mofeing · 2024-06-05T21:55:00Z

mmm you shouldn't edit the make.jl file: building mlir-jl-tblgen should generate a LocalPreferences.toml file in the project root that bindings/make.jl uses.

I used Julia 1.10.4

jumerckx · 2024-06-05T22:26:05Z

... that bindings/make.jl uses

For some reason that doesn't seem to happen on my machine.

i.e., when I run

julia --project=bindings/ bindings/make.jl

I don't get updated dialect wrappers, e.g. the results operand in affine.for should've been renamed to results_, but I don't see this.
I thought the LocalPreferences.toml would have to be in the same directory as the package env, but moving it to bindings/ doesn't change anything.

I expect I'm still doing something wrong from my side. If you feel like getting this PR through without troubleshooting my setup feel free to push the generated code to this branch.

mofeing · 2024-06-06T06:41:49Z

Maybe you haven't updated the deps and bindings environments? I manage this envs separately.

Anyway, I've just regenerated the bindings.

mofeing · 2024-06-06T06:50:13Z

@jumerckx can you check out if my generated bindings are okay? I don't see much difference... Maybe I'm the wrong one 😅

jumerckx · 2024-06-06T07:20:54Z

I don't see much difference...

Yeah me neither :)
To me it looks like the local mlir-jl-tblgen wasn't used.

jumerckx · 2024-06-06T08:06:05Z

src/Dialects/15/Affine.jl

-    operands = Value[operand_0..., ]
-    owned_regions = Region[region, ]
+function for_(
+    operand_0::Vector{Value}; results::Vector{IR.Type}, region::Region, location=Location()


Here, for example, the results argument isn't renamed anymore now.

jumerckx · 2024-06-06T13:32:56Z

Ok, I think I understand why MLIR14 is failing. The mlir-jl-tblgen tool is not version agnostic while it probably should be.
When generating dialect wrappers with a too recent mlir-jl-tblgen version, it fails, and when trying to build mlir-jl-tblgen with an older version the build fails because some import has been renamed in the meantime.
The Haskell folks seem to only support one MLIR version but they made this update: google/mlir-hs@ae26392.
I don't really know how to make the C++ version agnostic, though.

jumerckx · 2024-06-06T20:03:54Z

For LLVM/MLIR14, doing everything using Julia 1.9 just works.
For all the later versions, it seems OK to just use one Julia version (1.10 in my case).

This pr should be good to go now!
(I'm leaving formatting to #76)

…ywords` list

This reverts commit 752bdd9.

jumerckx commented Jun 6, 2024

View reviewed changes

jumerckx and others added 8 commits June 6, 2024 22:15

add local variables used in the generated function to the `reservedKe…

d1f8c6b

…ywords` list

changed API bindings?

610aebd

updated dialect bindings

d7ed635

ignore structs when generating API wrappers (these are wrapped manually)

e733dbf

newly generated API wrappers

e8d9201

Regenerate bindings

bf0b67c

Revert "Regenerate bindings"

e150d57

This reverts commit 752bdd9.

LLVM14 dialect wrappers (had to be generated using Julia 1.9)

e622bf1

jumerckx force-pushed the jm/generator-conflicts branch 2 times, most recently from 42c78a3 to e622bf1 Compare June 6, 2024 20:20

mofeing merged commit 67d3a33 into main Jun 6, 2024
6 checks passed

mofeing deleted the jm/generator-conflicts branch June 6, 2024 21:05

jumerckx mentioned this pull request Jul 23, 2024

handle multiple optional operands #81

Merged

jumerckx mentioned this pull request Oct 13, 2024

Generate bindings for MLIR 19 #82

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix operand names conflicting with local variables in the generated functions #74

fix operand names conflicting with local variables in the generated functions #74

jumerckx commented Jun 2, 2024

jumerckx commented Jun 2, 2024

jumerckx commented Jun 4, 2024

mofeing commented Jun 5, 2024 •

edited

Loading

jumerckx commented Jun 5, 2024

mofeing commented Jun 5, 2024

jumerckx commented Jun 5, 2024

mofeing commented Jun 5, 2024

jumerckx commented Jun 5, 2024

mofeing commented Jun 5, 2024

jumerckx commented Jun 5, 2024

mofeing commented Jun 5, 2024

jumerckx commented Jun 5, 2024

mofeing commented Jun 6, 2024

mofeing commented Jun 6, 2024

jumerckx commented Jun 6, 2024

jumerckx Jun 6, 2024 •

edited

Loading

jumerckx commented Jun 6, 2024

jumerckx commented Jun 6, 2024 •

edited

Loading

fix operand names conflicting with local variables in the generated functions #74

fix operand names conflicting with local variables in the generated functions #74

Conversation

jumerckx commented Jun 2, 2024

jumerckx commented Jun 2, 2024

jumerckx commented Jun 4, 2024

mofeing commented Jun 5, 2024 • edited Loading

jumerckx commented Jun 5, 2024

mofeing commented Jun 5, 2024

jumerckx commented Jun 5, 2024

mofeing commented Jun 5, 2024

jumerckx commented Jun 5, 2024

mofeing commented Jun 5, 2024

jumerckx commented Jun 5, 2024

mofeing commented Jun 5, 2024

jumerckx commented Jun 5, 2024

mofeing commented Jun 6, 2024

mofeing commented Jun 6, 2024

jumerckx commented Jun 6, 2024

jumerckx Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

jumerckx commented Jun 6, 2024

jumerckx commented Jun 6, 2024 • edited Loading

mofeing commented Jun 5, 2024 •

edited

Loading

jumerckx Jun 6, 2024 •

edited

Loading

jumerckx commented Jun 6, 2024 •

edited

Loading