This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Fix verification order of nested ops.
ClosedPublic

Authored by Chia-hungDuan on Mar 30 2022, 5:03 PM.

Details

Summary

In order to increase parallism, certain ops with regions and have the
IsIsolatedFromAbove trait will have their verification delayed. That
means the region verifier may access the invalid ops and may lead to a
crash.

Diff Detail

Event Timeline

Chia-hungDuan created this revision.Mar 30 2022, 5:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2022, 5:03 PM
Chia-hungDuan requested review of this revision.Mar 30 2022, 5:03 PM

Is there a test we can add that highlights the bug?

Add test cases

@rriddle, a gentle ping, what do you think about the tests?

rriddle accepted this revision.Apr 14 2022, 7:03 PM
rriddle added inline comments.
mlir/test/lib/Dialect/Test/TestDialect.cpp
1310

Why do we need this remark? (same below)

1321

Can we do mlir::verify on this just to be explicit?

This revision is now accepted and ready to land.Apr 14 2022, 7:03 PM
Chia-hungDuan marked an inline comment as done.Apr 14 2022, 9:12 PM
Chia-hungDuan added inline comments.
mlir/test/lib/Dialect/Test/TestDialect.cpp
1310

These are used for indicating the verify()/verifyRegion() has executed and ensure the invocation order of all verifiers is sustained.

For example, one of the test cases is, we expect seeing a verification failure in nested op and we also expect that the verify() should be executed. Then we don't miss the case like, we've verified non region related traits but forget to call Op::verify(). The same as verifyRegion().

Address review comments

This revision was landed with ongoing or failed builds.Apr 14 2022, 9:43 PM
This revision was automatically updated to reflect the committed changes.