Page MenuHomePhabricator

[MLIR][PDLInterp] Define new ops in PDLInterp to support commutativity
Needs ReviewPublic

Authored by srishti-pm on Feb 1 2022, 1:58 AM.

Details

Reviewers
rriddle
Summary

Defined 4 new ops in the PDLInterp dialect to help support
commutativity. The ops are as follows:

  1. pdl_interp.choose_range: This op performs a nondeterministic choice

of a range of PDL Values from a list of ranges.

  1. pdl_interp.get_item: This op gets an operand at index from a range of

values.

  1. pdl_interp.get_permutations: This op gets the permutations of given

list of PDL Values.

  1. pdl_interp.is_commutative: This op checks if a positional value is a

commutative operation.

Co-authors: Srishti Srivastava, Prateek Gupta

Signed-off-by: Srishti Srivastava <srishti.srivastava@polymagelabs.com>

Diff Detail

Event Timeline

srishti-pm created this revision.Feb 1 2022, 1:58 AM
srishti-pm requested review of this revision.Feb 1 2022, 1:58 AM
srishti-pm updated this revision to Diff 404868.Feb 1 2022, 3:03 AM
srishti-pm edited the summary of this revision. (Show Details)

Correct a typo in the commit summary.

Mogball requested changes to this revision.Feb 2 2022, 3:06 PM

Can you add lit tests for these new ops?

This revision now requires changes to proceed.Feb 2 2022, 3:06 PM

Thanks! Will get to reviewing this stack in the next day or two.

sfuniak added inline comments.Feb 3 2022, 4:04 AM
mlir/include/mlir/Dialect/PDLInterp/IR/PDLInterpOps.td
377–394

This is a duplicate of pdl_interp.foreach. Please remove this op and generalize pdl_interp.foreach if needed.

749–766

This is a duplicate of pdl_interp.extract; please remove.

1034–1051

Do we still need this op? As discussed earlier, I thought that whether or not an operation is commutative should be specified in the pdl.operation. If you do not want to force commutativity in pdl.operation, care must be taken to avoid the exponential blowup of the generated pdl_interp code.

Can you add lit tests for these new ops?

Sure, I'll do that. There is yet a consensus to be reached about which ops we add here. Will add their lit tests once that consensus is reached.

Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 10:50 AM
srishti-pm added inline comments.Mar 2 2022, 11:00 AM
mlir/include/mlir/Dialect/PDLInterp/IR/PDLInterpOps.td
377–394

Thanks Stano! I'm working on this.

749–766

Sure, working on it.

1034–1051

How would you advice this to be specified in the pdl.operation? Also, could you kindly let me know the following:

If my pattern looks like this in PDL:
%c = pdl.operation "foo.op" commutative (%a, %b : !pdl.value, !pdl.value) ->(%type : !pdl.type)
pdl.rewrite %c with "rewriter",
then how would you want its PDL_Interp to look like?

srishti-pm marked 3 inline comments as not done.Mar 4 2022, 2:34 AM
srishti-pm added inline comments.
mlir/include/mlir/Dialect/PDLInterp/IR/PDLInterpOps.td
749–766

@sfuniak, is it okay to generalize the pdl_interp.extract op? It seems like they aren't really exact duplicates and the pdl_interp.extract op might need some changes.

srishti-pm marked an inline comment as not done.May 16 2022, 8:18 PM
This revision now requires review to proceed.May 16 2022, 8:20 PM