Table op lowering to linalg.generic for both i8 (behaves like a gather) and a
pair of gathers with a quantized interpolation.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Sorry for the late review...
mlir/lib/Conversion/TosaToLinalg/TosaToLinalg.cpp | ||
---|---|---|
1391–1393 | This is not a template class, so we don't need template here. | |
1399–1401 | Can we move this above *ElementTy declarations? It would look more straight-forward to me. | |
1401 | I think probably say require inputTy to have static shape? Meaning it is a requirement for matching. | |
1405 | [optional] I personally would use ArrayRef<Value>{} | |
1418 | I would rather create a block manually than use nested builder. Because
I will use nested builder if it short. If you are going to use nested body builder, I recently learned that there is ImplicitLocOpBuilder b(nextedLoc, nestedBuilder);. With the builder, you no longer need to pass the loc variable. It would look much simpler to me if you use ImplicitLocOpBuilder. | |
1419–1420 | I think we can relax the condition to be "if all the element types are integer type and they have the same bitwidth? | |
1435 | I feel this could be nestedLoc, nestedBuilder.getI32IntegerAttr(32768) Same for below | |
1478 | nit: there are two space right after +, delete one. | |
1496 | drop punctuation at the send as these are supposed to be sentence fragments | |
1531–1533 | not need to change in this patch. I'm the one who also prefer wrap clang-format on/off here, so we wont see this kind of change... | |
mlir/test/Conversion/TosaToLinalg/tosa-to-linalg.mlir | ||
827–828 | One tip (because I'm not sure if this is a convention). There was a discussion long time ago for capturing % in variables. We ended with no because we can avoid {{\[}}. E.g., if you capture the variable without %, you can write // CHECK: %[[IDX_CAST:.+]] = index_cast %[[IDX]] ... // CHECK: %[[BASE:.+]] = tensor.extract %arg1[%[[IDX_CAST]]] | |
839 | delete trailing blank lines |
This is not a template class, so we don't need template here.