[mlir][openacc] Fix build after D148389

[razvanlupusoru]

Summary

Buildbot reported undefined references to LLVM dialect and Memref
dialect. The issue is that OpenACC dialect now depends on those
(since it attaches interface to the types) but the cmake file
did not explicitly add those dependencies.

[clementval]

LGTM

[aartbik]

do you also have a bazel equivalent for your recent changes
(I am trying to get that working again, but if you have something, that would help...)

[razvanlupusoru]

In D148673#4279019, @aartbik wrote:

do you also have a bazel equivalent for your recent changes
(I am trying to get that working again, but if you have something, that would help...)

Thank you for trying to get that working. I am not familiar with the bazel build - I am just investigating now to understand what I may have broken.

[razvanlupusoru]

So I added type interfaces for OpenACC. So the following should be added to the mlir/BUILD.bazel file:

gentbl_cc_library(
    name = "OpenACCTypeInterfacesIncGen",
    strip_include_prefix = "include",
    tbl_outs = [
        (
            ["-gen-type-interface-decls"],
            "include/mlir/Dialect/OpenACC/OpenACCTypeInterfaces.h.inc",
        ),
        (
            ["-gen-type-interface-defs"],
            "include/mlir/Dialect/OpenACC/OpenACCTypeInterfaces.cpp.inc",
        ),
    ],
    tblgen = ":mlir-tblgen",
    td_file = "include/mlir/Dialect/OpenACC/OpenACCTypeInterfaces.td",
    deps = [":OpenACCOpsTdFiles"],
)

[razvanlupusoru]

Also, the type interfaces .td file needs added to the list of OpenAccOpsTdFiles

td_library(
    name = "OpenAccOpsTdFiles",
    srcs = [
        "include/mlir/Dialect/OpenACC/AccCommon.td",
        "include/mlir/Dialect/OpenACC/OpenACCOps.td",
        "include/mlir/Dialect/OpenACC/OpenACCTypeInterfaces.td",
    ],
    includes = ["include"],
    deps = [":OpBaseTdFiles"],
)

[razvanlupusoru]

The dependencies in the "OpenACCDialect also need fixed:

cc_library(
    name = "OpenACCDialect",
    srcs = glob(
        [
            "lib/Dialect/OpenACC/IR/*.cpp",
            "lib/Dialect/OpenACC/IR/*.h",
        ],
    ),
    hdrs = glob([
        "include/mlir/Dialect/OpenACC/*.h",
    ]),
    includes = ["include"],
    deps = [
        ":IR",
        ":LLVMDialect",
        ":MemRefDialect",
        ":OpenACCOpsIncGen",
        ":OpenACCTypeInterfacesIncGen",
        ":Transforms",
        "//llvm:Support",
    ],
)

[razvanlupusoru]

@aartbik Would you like me to try to integrate the changes I described in the comments? It may take me longer to get a bazel build going if yes.

[aartbik]

I got that far, but it needs a dialect spec, as in

(
        ["-gen-type-interface-defs"],
        "include/mlir/Dialect/OpenACC/OpenACCTypeInterfaces.cpp.inc",
    ),
    (
        [
            "--gen-typedef-decls",
            "-typedefs-dialect=acc",
        ],
        "include/mlir/Dialect/OpenACC/OpenACCOpsTypes.h.inc",
    ),
    (
        [
            "--gen-typedef-defs",
            "-typedefs-dialect=acc",
        ],
        "include/mlir/Dialect/OpenACC/OpenACCOpsTypes.cpp.inc",
    ),

but after that (moving the build along) I am encountering name space issues

OpenACCOpsIncGen/mlir/Dialect/OpenACC/OpenACCOps.h.inc:482:35: error: no member named 'PointerLikeType' in namespace 'mlir::acc'
::mlir::TypedValue<::mlir::acc::PointerLikeType> getVarPtr();
                   ~~~~~~~~~~~~~^

[aartbik]

In D148673#4279080, @razvanlupusoru wrote:

@aartbik Would you like me to try to integrate the changes I described in the comments? It may take me longer to get a bazel build going if yes.

Yes, if you have a version that build, please send the bazel patch out
(that will make my life a bit easier integrating this into our personal workspace; which is outside your scope of responsibility, but,as you guessed, bazel based ;-)

[razvanlupusoru]

In D148673#4279086, @aartbik wrote:

In D148673#4279080, @razvanlupusoru wrote:

@aartbik Would you like me to try to integrate the changes I described in the comments? It may take me longer to get a bazel build going if yes.

Yes, if you have a version that build, please send the bazel patch out
(that will make my life a bit easier integrating this into our personal workspace; which is outside your scope of responsibility, but,as you guessed, bazel based ;-)

I do not have a version of the bazel build. And I didn't realize at first but I also need the bazel tool to do so - which is a bit harder to get it installed on the systems I am developing on. I can help with the patch if you are able to test (and I appreciate your help)

[razvanlupusoru]

In D148673#4279081, @aartbik wrote:

I got that far, but it needs a dialect spec, as in

(
        ["-gen-type-interface-defs"],
        "include/mlir/Dialect/OpenACC/OpenACCTypeInterfaces.cpp.inc",
    ),
    (
        [
            "--gen-typedef-decls",
            "-typedefs-dialect=acc",
        ],
        "include/mlir/Dialect/OpenACC/OpenACCOpsTypes.h.inc",
    ),
    (
        [
            "--gen-typedef-defs",
            "-typedefs-dialect=acc",
        ],
        "include/mlir/Dialect/OpenACC/OpenACCOpsTypes.cpp.inc",
    ),

but after that (moving the build along) I am encountering name space issues

OpenACCOpsIncGen/mlir/Dialect/OpenACC/OpenACCOps.h.inc:482:35: error: no member named 'PointerLikeType' in namespace 'mlir::acc'
::mlir::TypedValue<::mlir::acc::PointerLikeType> getVarPtr();
                   ~~~~~~~~~~~~~^

Do you have:

(
    ["-gen-type-interface-decls"],
    "include/mlir/Dialect/OpenACC/OpenACCTypeInterfaces.h.inc",
),

PointerLikeType is defined in OpenACCTypeInterfaces.h.inc

[aartbik]

(
    ["-gen-type-interface-decls"],
    "include/mlir/Dialect/OpenACC/OpenACCTypeInterfaces.h.inc",
),

Yeah, I have that, otherwise the file would not be generated. But somehow, the namespace is not right....

[razvanlupusoru]

First attempt at a patch to fix bazel build:

accbazel.patch1 KBDownload

[razvanlupusoru]

In D148673#4279097, @aartbik wrote:

(
    ["-gen-type-interface-decls"],
    "include/mlir/Dialect/OpenACC/OpenACCTypeInterfaces.h.inc",
),

Yeah, I have that, otherwise the file would not be generated. But somehow, the namespace is not right....

Can you paste the whole inclusion list? Whoever is including "mlir/Dialect/OpenACC/OpenACCOpsTypes.h.inc" also needs to include "mlir/Dialect/OpenACC/OpenACCTypeInterfaces.h.inc"

[razvanlupusoru]

In D148673#4279097, @aartbik wrote:

(
    ["-gen-type-interface-decls"],
    "include/mlir/Dialect/OpenACC/OpenACCTypeInterfaces.h.inc",
),

Yeah, I have that, otherwise the file would not be generated. But somehow, the namespace is not right....

OK - I think you are just missing this:

(
    ["-gen-typedef-decls"],
    "include/mlir/Dialect/OpenACC/OpenACCOpsTypes.h.inc",
),
(
    ["-gen-typedef-defs"],
    "include/mlir/Dialect/OpenACC/OpenACCOpsTypes.cpp.inc",
),

I made a mistake in the last patch I attached where I used a .cpp.inc instead of a .h.inc for the code above

[razvanlupusoru]

attempt 2:

accbazel2.patch1 KBDownload

[aartbik]

I appreciate your help, but this is what I had, and a lot of other stuff missing.
You e.g. don't include the OpenACCBase.td in the td file spec.
Also, you need to set attribute defs for the interfaces,
but that is where the namespace goes wrong...

[razvanlupusoru]

In D148673#4279129, @aartbik wrote:

I appreciate your help, but this is what I had, and a lot of other stuff missing.
You e.g. don't include the OpenACCBase.td in the td file spec.
Also, you need to set attribute defs for the interfaces,
but that is where the namespace goes wrong...

Ah! I didn't think I had to add all of the .td files which are merely intermediates for inclusion in other .td files (only those which are used to generate the .h.inc and .cpp.inc). That said, you are right I missed adding OpenACCOpsTypes.td in the inclusion list.

I can try to continue helping if it is useful. If yes, please paste the erroneous backtrace showing the unresolved type.

[aartbik]

I think I have a working version. I will send this out for review.

[razvanlupusoru]

In D148673#4279134, @aartbik wrote:

I think I have a working version. I will send this out for review.

Thank you!

[aartbik]

In D148673#4279142, @razvanlupusoru wrote:

In D148673#4279134, @aartbik wrote:

I think I have a working version. I will send this out for review.

Thank you!

My pleasure:

https://reviews.llvm.org/D148678

[mehdi_amini]

In D148673#4279142, @razvanlupusoru wrote:

In D148673#4279134, @aartbik wrote:

I think I have a working version. I will send this out for review.

Thank you!

@razvanlupusoru you may not have realized so far here, but Bazel support is absolutely not your responsibility in LLVM, it is perfectly fine to just answer: "sorry I don't anything about Bazel" and leave it there.
(I'm not trying to discourage you to support Bazel if you are interested in spending time in this area though)

[razvanlupusoru]

@mehdi_amini Thank you for your clarification. I did realize after I started investigating that bazel support is only in LLVM's peripheral tier support. But even though I didn't plan on using it, I wanted to do my best to help find a solution! :) As a community it is important to help each other and other times I may be the one needing the help. Anyway, I actually appreciate your comment because I understand you are trying to balance the burden of contributors.

mlir/lib/Dialect/OpenACC/CMakeLists.txt

 add_mlir_dialect_library(MLIROpenACCDialect
   IR/OpenACC.cpp
 
   ADDITIONAL_HEADER_DIRS
   ${MLIR_MAIN_INCLUDE_DIR}/mlir/Dialect/OpenACC
 
   DEPENDS
   MLIROpenACCOpsIncGen
   MLIROpenACCEnumsIncGen
   MLIROpenACCAttributesIncGen
   MLIROpenACCTypeInterfacesIncGen
 
   LINK_LIBS PUBLIC
   MLIRIR
+  MLIRLLVMDialect
+  MLIRMemRefDialect
   )