JuliaLabs / MLIR.jl

MIT License
56 stars 9 forks source link

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

Closed jumerckx closed 5 months ago

jumerckx commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

Awesome! Thanks @jumerckx

jumerckx commented 5 months ago

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 commented 5 months ago

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

jumerckx commented 5 months ago

I will try to run it in my computer tomorrow.

Cool, thanks!

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

mofeing commented 5 months ago

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

jumerckx commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

... 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 commented 5 months ago

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

Anyway, I've just regenerated the bindings.

mofeing commented 5 months ago

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

jumerckx commented 5 months ago

I don't see much difference...

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

jumerckx commented 5 months ago

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: https://github.com/google/mlir-hs/commit/ae26392d1069fe44a81cfd7cab3a03563808dad8. I don't really know how to make the C++ version agnostic, though.

jumerckx commented 5 months ago

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)