Page MenuHomePhabricator

[mlir][Transform] Make applyToOne return a DiagnosedSilenceableFailure

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



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".


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


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.


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.


Nit: "this the"


Which op?


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.




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.


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.


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.

Reworded with more detail.


Reworked that part.


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

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)


nicolasvasilache marked an inline comment as done.

Fix test.


ah ok, thanks!

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





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


Nit: would be nice to have a test.


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.