Page MenuHomePhabricator

[mlir] Add a new MutableOperandRange class for adding/remove operands
ClosedPublic

Authored by rriddle on Apr 26 2020, 7:40 PM.

Details

Summary

This class allows for mutating an operand range in-place, and provides vector like API for adding/erasing/setting. ODS now uses this class to generate mutable wrappers for named operands, with the name MutableOperandRange <operand-name>Mutable()

Depends On D78876

Diff Detail

Event Timeline

rriddle created this revision.Apr 26 2020, 7:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2020, 7:40 PM
rriddle updated this revision to Diff 260539.Apr 27 2020, 8:55 PM

Refactor into a new MutableOperandRange class.

rriddle retitled this revision from [mlir][ODS] Generate setter methods for named operands. to [mlir] Add a new MutableOperandRange class for adding/remove operands.Apr 27 2020, 8:55 PM
rriddle edited the summary of this revision. (Show Details)

First scan looks good :)

mlir/include/mlir/IR/Operation.h
214

How does this affects ops with AttrSizedOperandSegments? E.g., if someone were to call this function manually on an op that implements AttrSizedOperandSegments and inserts an operand in the middle of a segment, what happens?

mlir/include/mlir/IR/OperationSupport.h
699

Comments for these?

mlir/lib/IR/OperationSupport.cpp
317

Nit: s/proper// (proper always makes me think of something invalid, we are converting to an OperandRange and we wouldn't be creating an improper one so seems already captured)

328

Could we use AttrSizedOperandSegments's getOperandSegmentSizeAttr() instead? [I'm assuming the templating is what makes it ugly I know I did a "creative solution" equivalent to AttrSizedOperandSegments<void>::getOperandSegmentSizeAttr() in another such case]. But we already have this string in 2 places hardcoded. Could also be done in follow up

Could we alternatively pass in an optional NamedAttribute along with attrSizedOperandSgement in the constructor and at the call site query the attribute and reuse the name here?

rriddle updated this revision to Diff 260709.Apr 28 2020, 11:30 AM
rriddle marked 5 inline comments as done.

Resolve comments

rriddle added inline comments.Apr 28 2020, 11:31 AM
mlir/include/mlir/IR/Operation.h
214

The methods on Operation don't know about the presence of AttrSizedOperandSegments and shouldn't have to IMO. It is much more efficient to let the users that know about this handling(generally ODS and the derived op) take care of it themselves. Otherwise, the cost of checking for the presence of that becomes costly. I have generally been trying to avoid creeping the presence of traits into Operation unless there is a clear win, either in memory size or performance.

rriddle updated this revision to Diff 260873.Apr 29 2020, 3:04 AM

Use relative diff when updating segment attributes

jpienaar accepted this revision.Apr 29 2020, 6:56 AM

Nice, thanks

This revision is now accepted and ready to land.Apr 29 2020, 6:56 AM
This revision was automatically updated to reflect the committed changes.