[DC] Add canonicalization patterns #5158
Conversation
5a89970 to
88ddf5a
Compare
a581f01 to
5fe81bf
Compare
There was a problem hiding this comment.
Very partial review before I head out for the day.
A question: do tokens and values have implicit fork semantics? A quick skim through these seem to imply that they do. For instance, you can't have two joins with all the same inputs without said input (token) having at least two consumers. Similarly for values (which are basically token + data if I understand correctly), it's not possible to have two identical unpacks unless you have implicit fork semantics.
lib/Dialect/DC/DCOps.cpp
Outdated
| @@ -121,8 +121,152 @@ FunctionType CallOp::getCalleeType() { | |||
| // JoinOp | |||
| // ============================================================================= | |||
|
|
|||
| class IdenticalJoinCanonicalizationPattern : public OpRewritePattern<JoinOp> { | |||
There was a problem hiding this comment.
Can you put these in a separate file?
There was a problem hiding this comment.
Also the folds to be consistent with CombFolds.cpp.
| def HasOneOperand : Constraint<CPred<"$_self.size() == 1">, "has one operand">; | ||
| def HasOneResult : Constraint<CPred<"$_self.size() == 1">, "has one result">; | ||
|
|
||
| def EliminateSimpleJoinPattern : Pat<(JoinOp $arg), (replaceWithValue $arg), |
There was a problem hiding this comment.
These could be folds, correct? There are a bunch of advantages to using folds rather than canonicalization patterns, which I'm pretty sure are the only way these can be used.
There was a problem hiding this comment.
Bear in mind that we're gonna want to do constant propagation analysis which uses folds. Dunno if that applies here -- just a general comment.
There was a problem hiding this comment.
These have been converted to folds.
9bd020d to
6ffd187
Compare
6c5f33b to
a81d295
Compare
6ffd187 to
4b77a0a
Compare
Yep - i should add that to the rationale. Fork semantics, as well as sink semantics (i.e. values with 0 uses are legal) are implicit in the IR. Some lowering (e.g. to hardware) then have an additional requirement that this is made explicit. I think this is a fair design point, seeing as it makes writing canonicalization a fair bit easier, not having to peek through forks everywhere, and then later (if you indeed need forks in your IR), you can easily materialize those using #5159. |
a81d295 to
6404ef2
Compare
|
Will remove the "duplicate pack" canonicalization - this is just CSE. Furthermore, join canonicalization can be simplified by marking it as commutative. |
| @@ -118,8 +304,52 @@ LogicalResult UnpackOp::inferReturnTypes( | |||
| // PackOp | |||
| // ============================================================================= | |||
|
|
|||
| class EliminateMultiplePackPattern : public OpRewritePattern<PackOp> { | |||
There was a problem hiding this comment.
Does something like this exist upstream and would be automatically generated with some trait on pack? This seems like CSE to me.
There was a problem hiding this comment.
You're absolutely right - i actually already removed this canonicalization on my dev branch given that it's CSE'd automatically.
|
Sorry about the delinquent review. That's what happens when you tell me you're not in a rush to merge :P |
teqdruid
mikeurbach
lucas-rami
Dinistro
No description provided.