Skip to content

Conversation

@jerryyin
Copy link
Member

@jerryyin jerryyin commented Jun 24, 2022

Should hopefully address https://github.com/ROCmSoftwarePlatform/llvm-project-private/issues/524 but no guarantee.

This is based on the assumption that makefile used by dev-ops has a bug and used file to synchronize instead of target. The idea of the change is to insert a dummy generation step to force make wait on the previous custom command.

After insertion of this dummy target, the generation sequence of MLIRLinalgOpsIncGen is:

[1/8] Generating LinalgNamedStructuredOps.yamlgen.td, LinalgNamedStructuredOps.yamlgen.cpp.inc
[2/8] Generating dummy
[3/8] Building LinalgOpsTypes.cpp.inc...
[4/8] Building LinalgOpsDialect.h.inc...
[5/8] Building LinalgOps.cpp.inc...
[6/8] Building LinalgOpsDialect.cpp.inc...
[7/8] Building LinalgOps.h.inc...
[8/8] Building LinalgOpsTypes.h.inc...

In summary, the dummy target enforce a synchronization point to happen after the .cpp.inc and .yamlgen.td file generation; As well as a synchronization point to happen before the actual tablegen steps that follows it. In my observation, the generation of dummy file will block subsequent steps until it is done.

@jerryyin jerryyin requested review from krzysz00 and pcf000 June 24, 2022 15:36
@jerryyin jerryyin changed the title Adding dummy target to codegen Adding dummy target to LinalgNaedStructuredOps yamlgen Jun 24, 2022
@pcf000
Copy link
Contributor

pcf000 commented Jun 24, 2022

When I run the larger test with the add-dummy-dependency branch:
cmake -Bbuild -G'Unix Makefiles' . -DBUILD_FAT_LIBMLIRMIOPEN=1 cmake --build build -j 256
I see

[ 36%] Generating LinalgNamedStructuredOps.yamlgen.td, LinalgNamedStructuredOps.yamlgen.cpp.inc
[ 36%] Generating LinalgNamedStructuredOps.yamlgen.td, LinalgNamedStructuredOps.yamlgen.cpp.inc
[ 36%] Generating LinalgNamedStructuredOps.yamlgen.td, LinalgNamedStructuredOps.yamlgen.cpp.inc
[ 36%] Generating dummy
[ 36%] Generating dummy
[ 36%] Generating dummy

which is the familiar generate-three-times pattern. I can't tell at this level if they're being serialised. But it can't hurt to try it.

@jerryyin jerryyin merged commit 9ab1476 into miopen-dialect Jun 24, 2022
@jerryyin jerryyin deleted the add-dummy-dependency branch June 24, 2022 20:06
@jerryyin
Copy link
Member Author

There's probably nothing we can do about the 3 times calling of LinalgOdsGen, as three occurrences in its cmakefile seems to be correct. We'll give a shot on whether this can help and see if there's other opportunities if this doesn't fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants