This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Refactor the structure of the 'verifyConstructionInvariants' methods.
ClosedPublic

Authored by rriddle on Feb 19 2020, 3:18 PM.

Details

Summary

The current structure suffers from several problems, but the main one is that a construction failure is impossible to debug when using the 'get' methods. This is because we only optionally emit errors, so there is no context given to the user about the problem. This revision restructures this so that errors are always emitted, and the 'get' methods simply pass in an UnknownLoc to emit to. This allows for removing usages of the more constrained "emitOptionalLoc", as well as removing the need for the context parameter.

Fixes PR#44964

Diff Detail

Event Timeline

rriddle created this revision.Feb 19 2020, 3:18 PM
bondhugula accepted this revision.Feb 20 2020, 2:36 AM

Looks good to me.

mlir/include/mlir/IR/StorageUniquerSupport.h
28

Why is this returning an AttributeStorage pointer instead of a Location?

This revision is now accepted and ready to land.Feb 20 2020, 2:36 AM

Thanks for fixing this quickly! (Can you reference the bug report https://bugs.llvm.org/show_bug.cgi?id=44964 somewhere?)

rriddle marked 2 inline comments as done.Feb 20 2020, 9:48 AM
rriddle added inline comments.
mlir/include/mlir/IR/StorageUniquerSupport.h
28

We can't return Location here because it would require having the full declaration visible. Given that Locations are actually attributes, that isn't possible. To get around that, we can pass around the impl storage instead.

rriddle marked an inline comment as done.Feb 20 2020, 9:49 AM
rriddle edited the summary of this revision. (Show Details)Feb 20 2020, 9:57 AM

Thanks for the review Uday!

This revision was automatically updated to reflect the committed changes.