Page MenuHomePhabricator

[mlir][gpu] Fix logic error in D79508 computing number of private attributions.
ClosedPublic

Authored by whchung on May 28 2020, 3:30 PM.

Details

Reviewers
herhut
Summary

Fix logic error in D79508. The old logic would make the first check in
GPUFuncOp::verifyBody always pass.

Diff Detail

Event Timeline

whchung created this revision.May 28 2020, 3:30 PM
Herald added a project: Restricted Project. · View Herald Transcript
herhut requested changes to this revision.May 29 2020, 2:05 AM
herhut added inline comments.
mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
693

I don't think this should be in a verifier. getWorkgroupAttributions() is implemented by means of getNumWorkgroupAttributions, so this is not an invariant we need to check for each operation.

If we want to prevent this from regressing, we would need to write a unit test that shows the wrong behavior. You could specify the illegal case by using the generic syntax, which does not go through the parser but has to explicitly specify attributes. That should allow you to construct also illegal cases.

You can use mlir-opt -mlir-print-op-generic to look at the generic syntax.

This revision now requires changes to proceed.May 29 2020, 2:05 AM
whchung added inline comments.May 29 2020, 4:11 AM
mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
693

Thanks for the hint. Will revise the patch.

whchung updated this revision to Diff 267637.Jun 1 2020, 8:49 AM

Revise the fix and added positive and negative tests.

whchung edited the summary of this revision. (Show Details)Jun 1 2020, 8:50 AM
whchung marked 2 inline comments as done.
herhut added inline comments.Jun 1 2020, 11:40 AM
mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
680–681

This produces error messages that are not overly helpful. I would verify this is follows. We need at least getNumArguments() + getNumWorkgroupAttributions() many operands, as otherwise there are not enough. The getNumPrivateAttributions() does not need to be verified, as it is computed as the remaining operands that are left over.

So the only error case is when we do not have enough operands to satisfy function arguments and workgroup attributions.

whchung updated this revision to Diff 267699.Jun 1 2020, 12:20 PM

Address code review comments.

whchung marked an inline comment as done.Jun 1 2020, 12:20 PM
herhut accepted this revision.Jun 8 2020, 3:52 AM

Thanks! Please excuse the long delay due to me travelling.

This revision is now accepted and ready to land.Jun 8 2020, 3:52 AM
whchung closed this revision.Jun 8 2020, 9:26 AM

This patch was landed in commit 603b974cf7103766a0e5e4a0320fedb7c4b570f9.