[PATCH] D74172: [mlir][Linalg] Implement fusion of linalg.generic operation on tensors.
River Riddle via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 6 23:10:40 PST 2020
rriddle added inline comments.
================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgTraits.h:228
+ getInputs(),
+ [](Value v) { return v.getType().isa<RankedTensorType>(); }) &&
+ llvm::all_of(this->getOperation()->getResults(), [](Value v) {
----------------
nit: Can you separate the functor to avoid duplication?
================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp:328
+ // Verify that the producer and consumer are ops on tensors.
+ if (!producer.hasTensorSemantics() || !consumer.hasTensorSemantics()) {
+ return false;
----------------
Please remove all trivial braces.
================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp:374
+ auto t1 = producerArgIndexMap.compose(invProducerResultIndexMap);
+ // return is consumer loop -> producer arg tensor index, i.e. indexing_map for
+ // the producer argument in the fused operation.
----------------
Start sentences with a capital.
================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp:403
+ producerOp.operand_begin(),
+ std::next(producerOp.operand_begin(), producerOp.getNumInputs()));
+
----------------
nit: you could use getOperands()[producerOp.getNumInputs()]
================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp:427
+ SmallVector<Type, 2> fusedOpResultTypes;
+ llvm::for_each(consumerOp.getResults(), [&fusedOpResultTypes](Value v) {
+ fusedOpResultTypes.push_back(v.getType());
----------------
This seems like an unnecessary usage of for_each. Why not just write a for loop? You don't even need to do that really. You could just use getResultTypes() directly as it is just an ArrayRef<Type>
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74172/new/
https://reviews.llvm.org/D74172
More information about the llvm-commits
mailing list