Page MenuHomePhabricator

[mlir] Support operations with multiple results in slicing
ClosedPublic

Authored by ThomasRaoux on Jul 11 2020, 11:47 AM.

Details

Summary

Right now slicing would assert if an operation with multiple results is in the slice.

Diff Detail

Event Timeline

ThomasRaoux created this revision.Jul 11 2020, 11:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2020, 11:47 AM
bondhugula requested changes to this revision.Jul 12 2020, 8:46 AM
bondhugula added a subscriber: bondhugula.
bondhugula added inline comments.
mlir/lib/Analysis/SliceAnalysis.cpp
57

Nit: this shouldn't really be named ownerOp but a userOp. It's an owner op just for the OpOperand which isn't really in the picture.

173–174

Missing space - clang-format please.

174

Nit: Please avoid auto or use a better name (agree that this existed before). auto -> OpOperand or u -> opOperand.

174

You could just do an r.getUsers(). You only need the user ops.

This revision now requires changes to proceed.Jul 12 2020, 8:46 AM
ThomasRaoux marked 3 inline comments as done.

Address review comments.

ThomasRaoux marked an inline comment as done.Jul 12 2020, 10:24 PM
ThomasRaoux added inline comments.
mlir/lib/Analysis/SliceAnalysis.cpp
57

I'm not really changing this part of the code and the same naming is used several times so I would rather keep it consistent and send a NFC patch to change all the uses if you think it should be changed.

bondhugula added inline comments.Jul 12 2020, 10:35 PM
mlir/lib/Analysis/SliceAnalysis.cpp
57

If you are already touching a line, an NFC change to that same line (for example something like naming that improves readability) is completely fine to go along. We don't need a separate NFC patch (unless there are additional surrounding lines of code being modified).

nicolasvasilache accepted this revision.Jul 13 2020, 1:45 AM
nicolasvasilache added inline comments.
mlir/lib/Analysis/SliceAnalysis.cpp
57

+1 for s/ownerOp/userOp/g in this file.
You can just fold it as part of this revision as the changes are small.

174–175

nit - trivial braces here and below.

ThomasRaoux marked 2 inline comments as done.
mehdi_amini added inline comments.Jul 13 2020, 11:50 AM
mlir/lib/Analysis/SliceAnalysis.cpp
174

The comment about auto haven't been addressed: in general please spell the type when using auto does not improve readability (Value -> auto does not seem better to me, just like Operation * -> auto *)

ThomasRaoux marked 2 inline comments as done.Jul 13 2020, 1:02 PM
ThomasRaoux added inline comments.
mlir/lib/Analysis/SliceAnalysis.cpp
174

Replace auto.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 13 2020, 1:25 PM
This revision was automatically updated to reflect the committed changes.
ThomasRaoux marked an inline comment as done.