Extend op attrs with commutattive annotation#9479
Extend op attrs with commutattive annotation#9479mikepapadim wants to merge 2 commits intoapache:mainfrom
Conversation
mikepapadim
requested review from
MarisaKirisame,
ZihengJiang,
anijain2305,
areusch,
comaniac,
icemelon,
jroesch,
junrushao,
jwfromm,
mbrookhart,
merrymercy,
slyubomirsky,
tqchen,
vinx13,
wweic,
yzhliu and
zhiics
as code owners
mikepapadim
force-pushed
the
pattern_commutative_scopes
branch
from
6cf56f6 to
db634cf
Compare
mbs-octoml
left a comment
mbs-octoml
left a comment
There was a problem hiding this comment.
Suggestion to default the attrib to false and explicitly set to true at registration site for add & mul. Adding more if op == "add" etc logic seems to be going against the grain.
| /*! | ||
| * \brief Mark the operator as commutative to allow expr simplifications. | ||
| */ | ||
| using CommutativeOp = bool; |
| } | ||
| return false; | ||
| }; | ||
| auto is_commutative_op = [&get_op_node](const CallPatternNode* op_node) { |
There was a problem hiding this comment.
I was expecting this to look at the TCommutativeOp attribute of op_node.
| .add_type_rel("Identity", IdentityRel) \ | ||
| .set_attr<TOpPattern>("TOpPattern", kElemWise) \ | ||
| .set_attr<TOpIsStateful>("TOpIsStateful", false) \ | ||
| .set_attr<CommutativeOp>("CommutativeOp", false) \ |
There was a problem hiding this comment.
Do we need to put the commutativity property on unary and ops with 3+ arity?
I guess the downside is that then you'd need to check that arity == 2 before looking at the CommutativeOp attribute, or that the CommutativeOp attr is actually on the op.
| .add_type_rel("Broadcast", BroadcastRel) \ | ||
| .set_attr<TOpPattern>("TOpPattern", kBroadcast) \ | ||
| .set_attr<TOpIsStateful>("TOpIsStateful", false) \ | ||
| .set_attr<CommutativeOp>("CommutativeOp", isCommutativeOp(OpName)) \ |
There was a problem hiding this comment.
What if it's non-comm by default (so no attrib binding), and the registration of (for now) add & multiply explicitly set it to true.
|
closing due to inactivity, feel free to reopen if you'd like to merge |
5 participants
This PR extends the op
attrswith commutativity.This is required for the ongoing work to use the pattern language to express more general patterns.
@mbs-octoml @mbrookhart @jroesch @electriclilies