Page MenuHomePhabricator

[mlir] Add missing dependency to MLIRMlirOptMain
ClosedPublic

Authored by antiagainst on Tue, May 5, 9:42 AM.

Diff Detail

Event Timeline

antiagainst created this revision.Tue, May 5, 9:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, May 5, 9:42 AM
nicolasvasilache accepted this revision.Tue, May 5, 9:50 AM
This revision is now accepted and ready to land.Tue, May 5, 9:50 AM
jpienaar accepted this revision.Tue, May 5, 9:55 AM

Thanks!

This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Tue, May 5, 10:50 AM
mlir/tools/mlir-opt/CMakeLists.txt
47

Can you clarify which header included from mlir-opt.cpp depends on these?

antiagainst marked an inline comment as done.Tue, May 5, 12:30 PM
antiagainst added inline comments.
mlir/tools/mlir-opt/CMakeLists.txt
47

InitAllDialects.h. Do you want me to add as a comment to the code? Happy to send out a patch for it if so.

mehdi_amini added inline comments.Tue, May 5, 3:29 PM
mlir/tools/mlir-opt/CMakeLists.txt
47

I'd rather cut the dependency :)
I don't think calling into the initialization of dialect should require generated headers: this seems like a rather fundamentally static method.

It seems that this is due to template initialization, we could get away with an indirection, I'll look into this.

We saw some broken windows builds this morning, but I think there is a better fix. In general, I don't think everything should depend on intrinsics_gen.. In this case, MLIROptMain was missing a PUBLIC dependence on it's libraries (including Linalg) which was (I believe) the root of the problem.

In particular c296d2dc53d5c11ce0 should fix this.

We saw some broken windows builds this morning, but I think there is a better fix.  In general, I don't think everything should depend on intrinsics_gen..  In this case, MLIROptMain was missing a PUBLIC dependence on it's libraries (including Linalg) which was (I believe) the root of the problem.

Thanks for the pointer! Yeah that's better. Sent https://reviews.llvm.org/D79574 to remove the redundancy DEPENDS.