This is an archive of the discontinued LLVM Phabricator instance.

Don't store nullptrs in mlir::FuncOp::getAllArgAttrs' result
ClosedPublic

Authored by tpopp on Nov 22 2021, 3:57 AM.

Details

Summary

This method would create an ArrayRef full of nullptrs when there were no
argument attributes. This is problematic because this result could not
be passed to the FuncOp::build creator without causing a segfault.
Additionally, this is inconsistent with the the case that the argument
attribute list does not have to be the same length as the number of
arguments. Lastly, this removes error functionality that is inconsistent
with other attribute getters where only the entire return value might be
a nullptr rather than nullptrs inside a list.

Diff Detail

Event Timeline

tpopp created this revision.Nov 22 2021, 3:57 AM
tpopp requested review of this revision.Nov 22 2021, 3:57 AM
rriddle added inline comments.Nov 22 2021, 12:10 PM
mlir/include/mlir/IR/FunctionSupport.h
392–394

Can we just append a range of empty dictionary instead?

481–483

I imagine this would also need to be updated.

tpopp marked an inline comment as done.Nov 23 2021, 12:02 AM
tpopp added inline comments.
mlir/include/mlir/IR/FunctionSupport.h
392–394

Done.

tpopp marked an inline comment as done.Nov 23 2021, 12:03 AM

Was the patch updated?

tpopp updated this revision to Diff 389431.Nov 24 2021, 1:53 AM

Store empty attributes instead of nullptrs.

tpopp added a comment.Nov 24 2021, 1:55 AM

Was the patch updated?

Whoops 🙃

rriddle accepted this revision.Nov 24 2021, 10:43 AM

LGTM after resolving comments.

mlir/include/mlir/IR/FunctionSupport.h
393–394

Ah, I meant DictionaryAttr::get(getContext()). DictionaryAttr() is going to be a null attribute.

483–484

Same here.

This revision is now accepted and ready to land.Nov 24 2021, 10:43 AM
This revision was landed with ongoing or failed builds.Nov 25 2021, 6:12 AM
This revision was automatically updated to reflect the committed changes.