This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Mark LogicalResult as LLVM_NODISCARD
ClosedPublic

Authored by rriddle on Feb 1 2021, 5:58 PM.

Details

Summary

This makes ignoring a result explicit by the user, and helps to prevent accidental errors with dropped results. Marking LogicalResult as no discard was always the intention from the beginning, but got lost along the way.

Diff Detail

Event Timeline

rriddle created this revision.Feb 1 2021, 5:58 PM
rriddle requested review of this revision.Feb 1 2021, 5:58 PM

Perhaps a high level question: is the intention to just retain the current behavior or to check up on cases where currently not checked?

mlir/lib/Dialect/Affine/Utils/Utils.cpp
223

Why doesnt this return failure if this fails?

mehdi_amini accepted this revision.Feb 1 2021, 6:21 PM

Thanks!

This revision is now accepted and ready to land.Feb 1 2021, 6:21 PM
silvas accepted this revision.Feb 1 2021, 6:38 PM
silvas added inline comments.
mlir/include/mlir/Support/LogicalResult.h
16–17

Maybe add a blurb here about the fact that it is nodiscard, and how to explicitly discard it if needed (the (void) trick -- not obvious to most folks).

Also maybe document a best practice about only using it in contexts where it makes sense to always check the result?

rriddle marked 2 inline comments as done.Feb 2 2021, 2:18 PM

Perhaps a high level question: is the intention to just retain the current behavior or to check up on cases where currently not checked?

The intention for this revision is to be mostly NFC. I'll look at a few obvious fixes, but a lot of these seem to indicate a general lack of testing which would be better served by proper followups by the owners of the affected components. I plan to file bugs against these owners to look into/resolve the (void)s added for their components in this revision.

mlir/lib/Dialect/Affine/Utils/Utils.cpp
223

An applyPatternsAndFoldGreedily failure just means that it failed to converge when applying patterns. Returning failure in this case (and in most TBH) wouldn't make sense here given that failing to converge on the cleanup doesn't mean that the hoisting failed. I'm honestly considering moving the indication of non-convergence to an optional parameter or something, given the above.

rriddle updated this revision to Diff 320907.Feb 2 2021, 2:24 PM
rriddle marked an inline comment as done.

feedback

This revision was landed with ongoing or failed builds.Feb 4 2021, 3:10 PM
This revision was automatically updated to reflect the committed changes.