This is an archive of the discontinued LLVM Phabricator instance.

[mlir][linalg] Cleanup LinalgOp usage in fusion on tensors (NFC).
ClosedPublic

Authored by gysit on Jun 1 2021, 10:10 AM.

Details

Summary

Replace the uses of deprecated Structured Op Interface methods in FusionOnTensors.cpp. This patch is based on https://reviews.llvm.org/D103394.

Diff Detail

Event Timeline

gysit created this revision.Jun 1 2021, 10:10 AM
gysit requested review of this revision.Jun 1 2021, 10:10 AM
gysit added inline comments.Jun 1 2021, 10:14 AM
mlir/lib/Dialect/Linalg/Transforms/FusionOnTensors.cpp
252

@mravishankar shouldn't we use the indexing map of the consumer op operands used for fusion here as well as for the inputs?

mravishankar requested changes to this revision.Jun 1 2021, 11:06 AM

Overall looks ok to me. One comment in response to the question below.
Another minor nit: Why change signature from OpOperand & to OpOperand *. I prefer the former to the latter.

mlir/lib/Dialect/Linalg/Transforms/FusionOnTensors.cpp
102

If you are using OpOperand you dont need to send the consumer, consumerIdx or producer. You should be able to get the consumer with getOwner(), consumerIdx with getOperandNumber() and producer with get().getDefiningOp()

252

(things have changed here, so my recollection of this is based on what I originally wrote)

  • The order of operands for the fused op is [<consumer operands before the fused operand>, <producer operands>, <consumer operands after fused operands>] . For the <producer operands> you need to compute the indexing map based on the consumer op iteration space. The method here getIndexingMapOfProducerOperandsInCoordinatesOfFusedOp basically does that. For that you need the indexing map of the fused operand in the consumer and the indexing map of the operand in the producer. (see logic in this method to explain how that works).
  • I am actually not sure why there is fusion with init_tensor. That really should never happen. I would strongly suggest dropping this if condition. All tests that fail are effectively doing something that should be illegal.

So answer to your specific question might be, drop this if cause it shouldnt exist. I think the similar logic above should be fine.

This revision now requires changes to proceed.Jun 1 2021, 11:06 AM
mravishankar accepted this revision.Jun 1 2021, 11:56 AM

Had offline chat about resolving issues.

  1. Dropping the fusion at init_tensor will be done as a follow up.
  2. Use of pointer is the convention.

LGTM!

This revision is now accepted and ready to land.Jun 1 2021, 11:56 AM
gysit updated this revision to Diff 349184.Jun 1 2021, 11:59 PM

Address comment.

gysit updated this revision to Diff 349198.Jun 2 2021, 1:40 AM
gysit marked an inline comment as done.

Adapt casts in new code.

gysit updated this revision to Diff 349231.Jun 2 2021, 4:46 AM

Fix llvm::enumerate issue.