Page MenuHomePhabricator

[mlir] Add missing dependencies
AcceptedPublic

Authored by stephenneuendorffer on Oct 8 2020, 11:55 AM.

Details

Summary

This should solve some build errors using cmake + 'make'
This also continues some previous refactoring of the LLVM dialect to remove
dependence on the OpenMP dialect, which did not completely incorporate changes
to the header structure.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald Transcript
stephenneuendorffer requested review of this revision.Oct 8 2020, 11:55 AM
rriddle accepted this revision.Oct 8 2020, 11:59 AM
rriddle added inline comments.
mlir/lib/Conversion/LinalgToLLVM/CMakeLists.txt
9 ↗(On Diff #297028)

Why is this necessary? I wouldn't expect anything here to depend on Linalg passes being generated.

This revision is now accepted and ready to land.Oct 8 2020, 11:59 AM

Giving it a try, although I'll be able to see the result morning GMT.

Thanks for your help!

stephenneuendorffer marked an inline comment as done.Oct 8 2020, 1:54 PM
stephenneuendorffer added inline comments.
mlir/lib/Conversion/LinalgToLLVM/CMakeLists.txt
9 ↗(On Diff #297028)

LinalgToLLVM.cpp includes Passes.h, but it seems like that is unnecessary, so I've removed it.

stephenneuendorffer marked an inline comment as done.

It has taken more time than I expected as building with make -j1 is the best way to ensure that the potential problem with generated headers can be replicated. I've tried the latest iteration of this patch and I can confirm that the problem has gone and this patch is an acceptable solution.

make -j1 only enforces a linear ordering, but does not guarantee that the dependencies are correct. You actually have more chances to hit a missing dependency with maximum parallelism.

In general missing dependencies for a target can be reproduced consistently by building only this target alone make <path to .o>

mlir/lib/Target/CMakeLists.txt
55

Which config is failing without linking this?

stephenneuendorffer marked an inline comment as done.Oct 9 2020, 9:11 PM
stephenneuendorffer added inline comments.
mlir/lib/Target/CMakeLists.txt
55

I added it because of the header dependence on ConvertToLLVMIR, but it could be handled by the transitive dependence on MLIRTargetLLVMIRModuleTranslation, I suppose. I don't recall if I saw a specific failure here.

pawelo12345678 accepted this revision.Oct 11 2020, 10:24 AM