Depends On D93077
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Looks good!
Making a note here that I locally had to do 2 things for fusion on tensors:
- change some logic in getProducerOfTensor to properly handle scf.for iter_args:
static void getProducerOfTensor(Value tensor, LinalgOp &producer, unsigned &outputIndex) { while (true) { if (!tensor.getType().isa<RankedTensorType>()) return; if (auto linalgOp = tensor.getDefiningOp<LinalgOp>()) { producer = linalgOp; outputIndex = tensor.cast<OpResult>().getResultNumber(); return; } if (auto subTensorOp = tensor.getDefiningOp<SubTensorOp>()) { tensor = subTensorOp.source(); continue; } if (auto blockArg = tensor.dyn_cast<BlockArgument>()) { if (auto forOp = dyn_cast<scf::ForOp>(blockArg.getOwner()->getParentOp())) { tensor = *(forOp.getIterOperands().begin() + (blockArg.getArgNumber() - 1)); // account for iv continue; } } return; } }
- fix an indexing error at the beginning of fuseProducerOfTensor
Optional<FusionInfo> mlir::linalg::fuseProducerOfTensor(OpBuilder &b, LinalgOp consumer, unsigned consumerIdx) { Value inputTensor = consumerIdx < consumer.getNumInputs() ? consumer.getInput(consumerIdx) : consumer.getInitTensor(consumer.getNumInputs() - consumerIdx); LinalgOp producerOp; unsigned producerIdx; getProducerOfTensor(inputTensor, producerOp, producerIdx); if (!producerOp) return llvm::None;
I don't have a good setting to flush these changes at this time but signal boosting them here so you have them in case you need it in the future.
If not I'll prob hit the case again in a few more weeks and will deal with it then.
mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp | ||
---|---|---|
741 | Now would also be a good time to refactor op + idx into just OpOperand which is a more idiomatic Value that also tracks its operand index. |
Added this to this change itself.
mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp | ||
---|---|---|
741 | Good point. I will look into this since the dependent changes might need some time to resolve. |
mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp | ||
---|---|---|
741 | The latest changes uses OpOperand * to track dependencies. Wasnt sure how to adapt that to acsount for OpResult as well. Couple of things that I was thinking off but wasnt sure if those make sense
2)std::pair<OpOperand *, OpResult> this would work, but it probably needs a reimplementation of LinalgDependenceGraphElem to have nice interface to get the operation and operand/result number. Please let me know which would be better. Happy to do either. |
mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp | ||
---|---|---|
353 | s/For the now/For now | |
354–358 | If this is for consumer with buffer semantics only, we should keep the assertion. | |
355 | s/LinalgDependenceGraphElem/dependenceGraph I think we typically use the variable name instead of the type. (it would confuse if there are two variables in the same type.) | |
742–743 | I think this comment is for later statements, ie Value value = consumer.getOperation()->getOperand(consumerIdx); ....? If so, let's move it two lines below. | |
mlir/test/Dialect/Linalg/fusion-sequence.mlir | ||
61 | let's add three more spaces to align it if you intend to modify this... |
Rebase
mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp | ||
---|---|---|
742–743 | With the rebase, the comment is at the right place. |
mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp | ||
---|---|---|
357–360 | If we are going to check consumerOp, the above code should be dyn_cast. Otherwise, it results in a segfault. | |
744–746 | ditto, should be dyn_cast. Another point here is, we do assertion for buffers, but return llvm::None for tensors. I think this should be consistent. Let's make them both assert or return llvm::None? |
Rebase on top of change to LinalgDependenceGraph that accounts for
tracking dependence through result values.
s/For the now/For now