This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Transform] Make applyToOne return a DiagnosedSilenceableFailure
ClosedPublic

Authored by nicolasvasilache on Jul 6 2022, 3:49 AM.

Details

Summary

This revision revisits the implementation of applyToOne and its handling
of recoverable errors as well as propagation of null handles.
The implementation is simplified to always require passing a vector<Operation*>
in which the results are returned, resulting in less template instantiation magic.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2022, 3:49 AM
nicolasvasilache requested review of this revision.Jul 6 2022, 3:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2022, 3:49 AM
nicolasvasilache retitled this revision from [mlir][Transform][WIP] MAake applyToOne return a DiagnosedSilenceableFailure to [mlir][Transform] Make applyToOne return a DiagnosedSilenceableFailure.
nicolasvasilache edited the summary of this revision. (Show Details)

Finish the impl.

Add support for list of diagnostics.

ftynse requested changes to this revision.Jul 7 2022, 2:33 AM

I think the entire null pointer mess should remain in the internal of applyToOne, i.e., if nulls are returned, just filter them out before calling results.set in the apply implementation by TransformEachTrait. The failure state is already propagated, so there is no reason to know about nulls above that. Furthermore, this will fail the moment two distinct results have nulls in them because it will create a conflict in the "payload op -> handle" mapping that expects only one handle per payload op and there will be multiple handles to null.

Even more, I don't think we want to expose the "null handle" idea to the op documentation. This is an implementation detail. It is also a handle pointing to some nulls and some ops, not a null handle AFAICS. We can instead explain the tri-state failure and describe the failure mode along the lines of "if the transformation fails to apply to a subset of payload IR ops associated with the operand handle, the resulting handles will be associated only with the payload IR operations produced by the successful applications; the overall result will be a silenceable failure that the containing operation may chose to silence or report".

mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h
91

Nit: I suppose we want to separate different messages by newlines or something.

135

I'd rather consider a SmallVector<Diagnostic, 1> here with the convention that empty vector means no silenceable diagnostic. Currently, we can have an empty vector set, which is interpreted as silenceable diagnostic, but doesn't have any meaningful message.

599

It is unclear what it should put in the vector in the normal case of success, and whether it can assume the vector to be empty or not.
Also, a better explanation of the convention "fill with nulls and return success" is desirable.

600

Nit: "this the"

602

Which op?

610–613

I'd mention that in the case of silenceable failure, the transformation will still be applied to the following target ops, but definite failures are propagated immediately.

757

Micro-nit.

763–769

Why changing the severity? Silenceable diagnostics from individual applications are likely errors, so this should be too. If the caller doesn't silence them, it will still be interpreted as failure.

824
837–840

Because of the change above from "not success" to "is definite failure", this now looks incorrect in the following case. A transform op with legitimately zero results returns a silenceable failure and zero results. This will now turn that silenceable failure into a success. instead of collecting the failure message properly.

872

Nit: this feels like a note that can be attached to the original message.

This revision now requires changes to proceed.Jul 7 2022, 2:33 AM
nicolasvasilache marked 11 inline comments as done.Jul 7 2022, 4:06 AM
nicolasvasilache added inline comments.
mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h
610–613

Reworded with more detail.

837–840

Reworked that part.

872

I reworked this to force applyToOne to always produce the expected number of results.
It can make them all null but if the right number is not present, this is an error.

ftynse added inline comments.Jul 7 2022, 4:36 AM
mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h
872

I meant specifically the part that starts with a newline can be attached as a note to the error. This is how these things usually look: the error message, and the note "did you mean foo?"

ftynse added a comment.Jul 7 2022, 4:37 AM

You seem have created separate review threads for the patches that were supposed to be addressing review in this one.

nicolasvasilache marked 3 inline comments as done.
nicolasvasilache edited the summary of this revision. (Show Details)

Rebase.

nicolasvasilache marked an inline comment as done.

Fix test.

mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h
872

ah ok, thanks!

ftynse accepted this revision.Jul 7 2022, 7:03 AM

Thanks!

mlir/include/mlir/Dialect/Linalg/TransformOps/LinalgTransformOps.td
30 ↗(On Diff #442889)

s/vectorize/decompose

mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h
764

Nit: I'd drop the "null handle" part from here, the user doesn't necessarily know such details.

853–860

Nit: would be nice to have a test.

mlir/test/Dialect/Transform/test-interpreter.mlir
412–413 ↗(On Diff #442889)

Ultra-nit: missing whitespace

This revision is now accepted and ready to land.Jul 7 2022, 7:03 AM
nicolasvasilache marked 2 inline comments as done.

Address last comments.

Improve error message with a note.

This revision was landed with ongoing or failed builds.Jul 7 2022, 7:32 AM
This revision was automatically updated to reflect the committed changes.