Page MenuHomePhabricator

Add IntegerAttr::verifyConstructionInvariants.
ClosedPublic

Authored by silvas on Jan 29 2020, 10:15 AM.

Details

Summary

This will help catch improper use of the MLIR API's. In particular, this
catches an error that was manifesting as nondeterministic assertion
failures (the nondeterminism was due to the failure happening only when the
StorageUniquer's DenseMap's probing happened to compare two specific
keys).

No test. The fact that all the existing tests pass with this additional
invariant gives confidence that it is correct/useful.

Diff Detail

Event Timeline

silvas created this revision.Jan 29 2020, 10:15 AM
rriddle accepted this revision.Jan 29 2020, 10:23 AM
rriddle added inline comments.
mlir/lib/IR/Attributes.cpp
294

nit: can you just return this directly?

308

nit: Can you add the widths as well to the message?

Note: emitOptionalError takes variadic arguments that are passed to the diagnostic

emitOptionalError(loc, "these are ", 3, " arguments to the diagnostic");

This revision is now accepted and ready to land.Jan 29 2020, 10:23 AM

Unit tests: pass. 62302 tests passed, 0 failed and 838 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

silvas updated this revision to Diff 241225.Jan 29 2020, 10:55 AM

Fix review feeback.

silvas marked 2 inline comments as done.Jan 29 2020, 10:56 AM

Fix review feedback.

Unit tests: pass. 62302 tests passed, 0 failed and 838 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

This revision was automatically updated to reflect the committed changes.