Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Codegen][Tuner] verifier for the default tuning spec #19525

Merged
merged 6 commits into from
Jan 3, 2025

Conversation

bangtianliu
Copy link
Contributor

@bangtianliu bangtianliu commented Dec 19, 2024

This PR adds the unit attribute iree_codegen.tuning_spec_with_default_entrypoint to indicate the default tuning spec (typically or user-provided tuning spec but can work in the same manner) must contain one named sequence operation marked with __kernel_config, also add the corresponding verification in verifyOperationAttribute function.

This PR is relevant to task in #19214: add a discardable attr verifier for entry points iree_codegen.tuning_spec_entrypoint

Context:
Jakub proposed two approaches for verifying the default tuning specification:

  1. Implement a dedicated pass for verification.
  2. Add a new attribute and update the verifyOperationAttribute function accordingly.

After careful consideration, we agreed on the second approach to avoid introducing an additional pass, ensuring a simple implementation.

Copy link
Member

@kuhar kuhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain what are the alternatives you considered and why you decided on this way of verifying the default specs? IIRC, we first discussed a dedicated verification pass.

Also, please update the documentation in https://iree.dev/reference/tuning/

@kuhar
Copy link
Member

kuhar commented Dec 19, 2024

There are test failures.

@bangtianliu
Copy link
Contributor Author

Can you explain what are the alternatives you considered and why you decided on this way of verifying the default specs? IIRC, we first discussed a dedicated verification pass.

Also, please update the documentation in https://iree.dev/reference/tuning/

During our discussion, you proposed two approaches: 1) use one separate pass to verify the default spec; 2) add one attribute and update the verifyOperationAttribute function. At the end, I think we both agree on second one, is my understanding wrong?

@kuhar
Copy link
Member

kuhar commented Dec 19, 2024

This is something that should be explained in the PR description for all the other folks who did not join the same meeting

@bangtianliu
Copy link
Contributor Author

This is something that should be explained in the PR description for all the other folks who did not join the same meeting

Ok, will add it in the PR description.

@bangtianliu
Copy link
Contributor Author

There are test failures.

Yeah, will work on fixing it.

@bangtianliu
Copy link
Contributor Author

bangtianliu commented Dec 19, 2024

In cases where both a user-provided tuning specification (tuning_spec_mmt_tile_and_fuse.mlir) and the default specification (iree_default_tuning_spec_gfx942.mlir) are used, the default spec part, after linking the two MLIR files, contains a named sequence operation with the symbol name __kernel_config_1 instead of __kernel_config. This behavior is due to the logic implemented here: LinkTuningSpecsPass.cpp, Line 110.

To address this which causes the ci errors, I updated the code from

SymbolTable::getSymbolName(namedSeqOp).getValue() ==
                   kKernelConfigSpecName;

to

SymbolTable::getSymbolName(namedSeqOp)
                  .getValue()
                  .contains(kKernelConfigSpecName);

@bangtianliu bangtianliu force-pushed the verify_default_ts branch 2 times, most recently from 8f31e38 to b2d4cd2 Compare December 19, 2024 13:36
@kuhar
Copy link
Member

kuhar commented Jan 2, 2025

contains a named sequence operation with the symbol name __kernel_config_1 instead of __kernel_config. This behavior is due to the logic implemented here: LinkTuningSpecsPass.cpp, Line 110.

Good observation, this needs to be handled.

To address this which causes the ci errors, I updated the code from

This fix is incorrect. It no longer verifies the there's the exact entry point with the default name. If the outermost entrypoint is anything __kernel_config, the tuning spec won't be applied. Instead, we need to drop the module-level attribute whenever we rename entrypoints during linking.

@bangtianliu bangtianliu requested a review from kuhar January 2, 2025 22:25
Signed-off-by: Bangtian Liu <[email protected]>
@bangtianliu bangtianliu requested a review from kuhar January 3, 2025 00:36
Copy link
Member

@kuhar kuhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bangtianliu bangtianliu merged commit e6ac016 into iree-org:main Jan 3, 2025
37 checks passed
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.

2 participants