Page MenuHomePhabricator

[mlir] Fix a build error and a warning in mlir
AbandonedPublic

Authored by isuruf on Jun 5 2020, 10:49 AM.

Details

Reviewers
silvas

Diff Detail

Event Timeline

isuruf created this revision.Jun 5 2020, 10:49 AM
Herald added a project: Restricted Project. · View Herald Transcript
silvas requested changes to this revision.Jun 5 2020, 11:00 AM
silvas added inline comments.
mlir/lib/Dialect/Shape/CMakeLists.txt
1

Why is this needed?

This revision now requires changes to proceed.Jun 5 2020, 11:00 AM
isuruf marked an inline comment as done.Jun 5 2020, 11:03 AM
isuruf added inline comments.
mlir/lib/Dialect/Shape/CMakeLists.txt
1

Because mlir_tablegen call in Line 3 will try to create the IR/ShapeCanonicalization.inc file without creating the IR directory.

@tpopp can you PTAL at this? Surely we aren't the first using DRR inside mlir core, and I didn't find evidence that this CMakeLists.txt annotation was needed for them. Wild guess, but perhaps we can put the ShapeCanonicalization.td in lib/ ?

mlir/Dialect/Vector/CMakeLists.txt doesn't seem to need this?

If this is critically breaking bots then we should revert the original patch. We should take some time to get to the bottom of this specific issue. I don't think that MAKE_DIRECTORY is the right solution here.

isuruf added a comment.Jun 5 2020, 1:35 PM

@silvas, mlir_tablegen(IR/ShapeCanonicalization.inc -gen-rewriters) is the only call in the MLIR project with a folder (IR) in it. Others look like mlir_tablegen(SPIRVEnums.h.inc -gen-enum-decls)

Maybe you could move to a slightly different directory structure and follow what other dialects are doing. Using a top-level CMakeLists.txt like

# lib/Dialect/Shape/CMakeLists.txt
add_subdirectory(IR)

and then add the dialect library generation directives in lib/Dialect/Shape/IR?
I think it would make sense to follow the same conventions as, say, the Linalg dialect.

tpopp added a comment.Jun 6 2020, 1:24 AM

First, sorry for the breakage.

If you want to revert the changes, for now you would need to revert both https://github.com/llvm/llvm-project/commit/0a554e607ff6247b79d1c4f184999750e5ad53b9 and https://github.com/llvm/llvm-project/commit/6aab70945915ef1d565f1146734416029549a5a9

I'm not able to reproduce it myself and don't see it in the buildbots, but assuming this works, https://reviews.llvm.org/D81328 sounds like a good final end state like Kayjukh suggested, but I could do this and undo the reverts after also.

(I'm not commenting on the DRR because I'm not sure what that stands for).

tpopp added a comment.Jun 6 2020, 2:08 AM

As more people are complaining, I'm going ahead and reverting the CLs that added and rely on this new file.