[water] promote the duplicate non-unit step verifier#1050
[water] promote the duplicate non-unit step verifier#1050
Conversation
Until now, we have only been verifying the absence of a second non-unit step in index expressions of read and write operations. Do so for every operation. The check is performed when verifying the hyperparameters attribute since hyperparameters are likely necessary to evaluate the step expression. Therefore, the verification will be skipped in absence of this attribute. This is not ideal, but the alternative is to have every single verifier look up in the region tree to find the hyperparameters, which isn't very efficient. A better, longer-term solution is to introduce a top-level wave kernel operation where hyperparameters are mandatory. Closes #1013. Signed-off-by: Alex Zinenko <git@ozinenko.com>
ftynse
changed the title
ftynse
requested review from
Copilot and
martin-luecke
and removed request for
Copilot
There was a problem hiding this comment.
Pull request overview
This PR promotes the duplicate non-unit step verifier for index expressions from being specific to read/write operations to applying to all Wave operations with an index attribute. Previously, the multi-step check was embedded in verifyIndexElementsPerThread (called only for read/write/selfIndex ops). Now it's centralized in a new verifyIndexStepsAtMostOneNonUnit function in WaveDialect.cpp, which runs for all ops during the wave.hyperparameters attribute verification.
Changes:
- New
verifyIndexStepsAtMostOneNonUnitfunction inWaveDialect.cppchecks all ops for multiple non-unit index steps (using hyperparameters). Also improves error messages by reporting the symbolic dimension name instead of its integer index. - Removed the inline multi-step check from
verifyIndexElementsPerThreadinWaveOps.cpp, which also drops the now-unused loop index variable. - Tests updated in
ops-invalid.mlirandpropagate-elements-per-thread.mlirto add hyperparameters where needed, update expected error messages, and add a new test case forwave.register.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
water/lib/Dialect/Wave/IR/WaveDialect.cpp |
Adds new verifyIndexStepsAtMostOneNonUnit function and invokes it for all ops during hyperparameter attribute verification |
water/lib/Dialect/Wave/IR/WaveOps.cpp |
Removes the duplicated multi-step check from verifyIndexElementsPerThread; replaces enumerate loop with simpler range loop |
water/test/Dialect/Wave/ops-invalid.mlir |
Updates read_index_multi_step and read_index_multi_step_eval tests for new error message format; adds write_index_multi_step_eval test |
water/test/Dialect/Wave/propagate-elements-per-thread.mlir |
Adds @multiple_non_unit_steps test; updates @index_multi_non_unit_step to remove now-unnecessary hyperparameters |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // ----- | ||
|
|
||
| // Two dimensions with non-unit steps; pass must report "expected only one non-unit". | ||
| // Use only register (write has its own verifier for multi-step index). |
There was a problem hiding this comment.
The comment on line 165 says "Use only register (write has its own verifier for multi-step index)." However, this PR removes the multi-step check from verifyIndexElementsPerThread in WaveOps.cpp (which was shared by the read and write op verifiers). The write op no longer has its own verifier for multi-step index — the check is now performed by verifyIndexStepsAtMostOneNonUnit in WaveDialect.cpp, which applies to all ops. The comment should be updated to reflect this change.
| // Use only register (write has its own verifier for multi-step index). | |
| // Use only register; multi-step index is now verified by the Wave dialect for all ops. |
There was a problem hiding this comment.
true, let's remove the stale comment
| // ----- | ||
|
|
||
| // Two dimensions with non-unit steps; pass must report "expected only one non-unit". | ||
| // Use only register (write has its own verifier for multi-step index). |
There was a problem hiding this comment.
true, let's remove the stale comment
| // Check the well-formedness of the index attribute (must have at most one | ||
| // non-unit dimension) and its correspondence with the explicit elements per | ||
| // thread, if provided, and with the number of elements in the vector type. | ||
| static LogicalResult |
There was a problem hiding this comment.
This comment is stale, too. This function does not check for " at most one
non-unit dimension" anymore
| auto dict = dyn_cast<DictionaryAttr>(elem); | ||
| if (!dict) | ||
| continue; | ||
|
|
||
| int nonUnitCount = 0; | ||
| for (const NamedAttribute &named : dict) { | ||
| auto mapping = | ||
| llvm::dyn_cast<wave::WaveIndexMappingAttr>(named.getValue()); |
There was a problem hiding this comment.
let's use either dyn_cast<...> or llvm::dyn_cast<...> consistently
| if (!stepValues || stepValues->size() != 1) | ||
| continue; |
There was a problem hiding this comment.
Do we have cases where stepValues->size() > 1?
I assume this would be malformed IR that would be caught by a later verifier, correct?
| diag.attachNote() << "second non-unit step dimension: " << i; | ||
| return diag; | ||
| } | ||
|
|
There was a problem hiding this comment.
The new code at this place looks like it might miss when we have multiple entries with non-unit steps now. I think it's worth adding a comment before the if indicating that this is caught by another verifier.
Until now, we have only been verifying the absence of a second non-unit
step in index expressions of read and write operations. Do so for every
operation. The check is performed when verifying the hyperparameters
attribute since hyperparameters are likely necessary to evaluate the
step expression. Therefore, the verification will be skipped in absence
of this attribute. This is not ideal, but the alternative is to have
every single verifier look up in the region tree to find the
hyperparameters, which isn't very efficient. A better, longer-term
solution is to introduce a top-level wave kernel operation where
hyperparameters are mandatory.
Closes #1013.