Page MenuHomePhabricator

[MLIR] Make commutative equivalence as an option
Needs ReviewPublic

Authored by clementval on Jul 11 2022, 4:47 AM.

Details

Summary

As discussed on D123492, there are some use cases downstream
where doing the equivalence on pointers is not desired. This patch
makes the equivalence checks configurable with a new flag so the previous
behavior is restored and the CSE pass can still make use of the pointers
equivalence on commutative operations.

Diff Detail

Event Timeline

clementval created this revision.Jul 11 2022, 4:47 AM
Herald added a project: Restricted Project. · View Herald Transcript
clementval requested review of this revision.Jul 11 2022, 4:47 AM

I don't think you answered a question I had in your previous revision: can we try instead to move this to a canonicalization: we could sort operands (we'd need some stable sort based on some numbering in the region maybe).

mlir/include/mlir/IR/OperationSupport.h
1201

This should mention how dangerous it is I think.

I don't think you answered a question I had in your previous revision: can we try instead to move this to a canonicalization: we could sort operands (we'd need some stable sort based on some numbering in the region maybe).

Is your idea to have a canonicalization that make commutative op operands ordering and then the CSE can just take over an eliminate the equivalent ops without special trick? Yeah I think that's a valid solution as well.

mlir/include/mlir/IR/OperationSupport.h
1201

I will update.

I don't think you answered a question I had in your previous revision: can we try instead to move this to a canonicalization: we could sort operands (we'd need some stable sort based on some numbering in the region maybe).

Is your idea to have a canonicalization that make commutative op operands ordering and then the CSE can just take over an eliminate the equivalent ops without special trick? Yeah I think that's a valid solution as well.

Yes that is what I have in mind.

I don't think you answered a question I had in your previous revision: can we try instead to move this to a canonicalization: we could sort operands (we'd need some stable sort based on some numbering in the region maybe).

Is your idea to have a canonicalization that make commutative op operands ordering and then the CSE can just take over an eliminate the equivalent ops without special trick? Yeah I think that's a valid solution as well.

Yes that is what I have in mind.

Ok. Sounds reasonable to me. I'm not sure when I can work on this but I'll give it a try.