[CMake] Add MLIRArithOpsInterfacesIncGen dependency

[rsetaluri]

See #5634.

[mikeurbach]

Beat me to it, I had just looked for where this might be missing myself. I think this is on the right track, but a couple are missing.

I believe you've searched for CIRCT libraries that link MLIRArithDialect, and added a DEPENDS on MLIRArithOpsInterfacesIncGen? I think that's a good clue, but not exactly what we need.

If I understand it correctly, the issue is really what libraries have #include "mlir/Dialect/Arith/IR/Arith.h", since Arith.h includes the files generated by MLIRArithOpsInterfacesIncGen. There are some libraries in CIRCT that don't actually link MLIRArithDialect, but do import Arith.h. There are also libraries in CIRCT that do link MLIRArithDialect, but do not import Arith.h.

Looking from the angle of what imports Arith.h, I would add the following:

lib/Conversion/AffineToLoopSchedule/CMakeLists.txt
lib/Conversion/LLHDToLLVM/CMakeLists.txt
lib/Dialect/DC/CMakeLists.txt
lib/Dialect/Handshake/CMakeLists.txt
lib/Dialect/Handshake/Transforms/CMakeLists.txt

I think you can also remove the following, which don't actually import Arith.h:

lib/Dialect/Calyx/CMakeLists.txt
lib/Dialect/Calyx/Transforms/CMakeLists.txt
lib/Dialect/ESI/CMakeLists.txt

[rsetaluri]

Thanks for auditing those, I'll update to the appropriate set.

Do I have the name wrong: https://github.com/llvm/circt/actions/runs/5602763622/jobs/10248467277?pr=5636#step:10:156 ?

[mikeurbach]

Do I have the name wrong

Hmmm, I used MLIRArithOpsInterfacesIncGen and it builds locally. I wonder if this is something about unified builds versus not. I build unified locally, but the failing CI uses the separate build of LLVM then CIRCT.

[rsetaluri]

Do I have the name wrong

Hmmm, I used MLIRArithOpsInterfacesIncGen and it builds locally. I wonder if this is something about unified builds versus not. I build unified locally, but the failing CI uses the separate build of LLVM then CIRCT.

Hmm...does cmake need to be made aware of that then?

[mikeurbach]

I'm not sure... there has to be a way to express this. I'll try to find other examples in the open source that make the two-phased builds work.

[rsetaluri]

I'm not sure... there has to be a way to express this. I'll try to find other examples in the open source that make the two-phased builds work.

@mikeurbach Any update on this? Happy to take a look at something I can reproduce if you can point me in the right direction. Thanks!

[mikeurbach]

I'm not sure yet what the right solution here is. It seems like the MLIRArithOpsInterfacesIncGen target isn't available in an out of tree build, but I don't understand why, since targets like MLIRArithDialect are. I looked at how torch-mlir handles out of tree builds versus circt, and it seems the same. I also don't see torch-mlir needing to depend on MLIRArithOpsInterfacesIncGen even though they import Arith.h in several libraries (example: https://github.com/llvm/torch-mlir/blob/main/lib/Conversion/TorchToTosa/CMakeLists.txt). So maybe adding a dependency on MLIRArithOpsInterfacesIncGen is not correct, and is a red herring.

I don't know how to reliably reproduce the original issue, but that seems to be the thing we really need to figure out. Does it keep happening in CI, or did we just get (un)lucky in the run at the top of this issue? What's weird is I can see the MLIRArithOpsIncGen was built, just not the MLIRArithOpsInterfacesIncGen: https://github.com/llvm/circt/actions/runs/5598847526/job/15165852121#step:6:1260.

EDIT: seems like the most recent nightly run succeeded, but I'm not sure if that was transient or it will continue to succeed.

[rsetaluri]

Does it keep happening in CI, or did we just get (un)lucky in the run at the top of this issue?

From what I can see, it seems very flaky both ways. My opinion is that we should keep an eye on when it passes and fails, but just having one successful run unblocks me. I imagine for actual tagged releases we can retry if it fails?

In the meantime I'll try to see if I can make sense of why MLIRArithOpsInterfacesIncGen isn't building...

[mikeurbach]

I imagine for actual tagged releases we can retry if it fails?

I think it's possible, but that will be painful. It does seem flaky, so we should get to the bottom of it. I wonder if declaring DEPENDS on the MLIRArithDialect instead would work.

[rsetaluri]

I imagine for actual tagged releases we can retry if it fails?

I think it's possible, but that will be painful. It does seem flaky, so we should get to the bottom of it. I wonder if declaring DEPENDS on the MLIRArithDialect instead would work.

Agreed. Let me try that in this PR

[rsetaluri]

@mikeurbach this seemed to work! I will try the action against this branch:

https://github.com/llvm/circt/actions/runs/5625831739

[mikeurbach]

Awesome, sounds good.

[rsetaluri]

@mikeurbach seems to be all good!

[mikeurbach]

This seems a little unusual, but I think it is our best bet to reliably squash the situation that caused the failures in CI. Given this plan, this PR itself looks good to me.