Conversation
gdehame
commented
Feb 10, 2025
Added a lowering to linalg for the torch.aten.col2im operation.
Added a unit test to verify the lowering.
1a2ce71 to
5158529
Compare
|
Hi @gdehame, thanks for the submission. I'm yet to review the code -- just leaving one high-level feedback. How does this op appear in the IR through the |
sahas3
left a comment
There was a problem hiding this comment.
I've taken a first pass and left some comments. Thanks!
There was a problem hiding this comment.
| if (!(col2imOp.getOutputSize().getDefiningOp() && | |
| isa<Torch::PrimListConstructOp>( | |
| col2imOp.getOutputSize().getDefiningOp()))) | |
| return failure(); | |
| Torch::PrimListConstructOp outputSizes = cast<Torch::PrimListConstructOp>( | |
| col2imOp.getOutputSize().getDefiningOp()); | |
| auto outputSizes = col2imOp.getOutputSize().getDefiningOp<Torch::PrimListConstructOp>(); | |
| if (!outputSizes) | |
| return failure(); |
There was a problem hiding this comment.
Here also similar suggestion about grabbing the def op at once and verifying it exists can be implemented as mentioned above.
There was a problem hiding this comment.
Similar comment here and elsewhere too.
| Type elementType = outputType.getElementType(); | ||
| Value outputBuffer = tensor::EmptyOp::create( | ||
| rewriter, col2imOp->getLoc(), | ||
| ArrayRef<int64_t>{outputType.getDimSize(0), outputType.getDimSize(1), |
There was a problem hiding this comment.
I think assumption here is that outputType dims 0 and 1 are static but that is not explicitly checked anywhere.
There was a problem hiding this comment.
These nested tertiary ops are hard to read (here and in other places). Can you break this down?
There was a problem hiding this comment.
This can be checked before starting to create any new ops in the IR and failure() can be returned for this unsupported case instead of an assert.
There was a problem hiding this comment.
integer/complex types are not being tested. I'd suggest adding e2e tests for testing those combinations
There was a problem hiding this comment.
I started doing so and realized the integer case is bugged due to using arith.constant to generate a 0 constant of possibly (un)signed int which is not allowed.
I'll work on this further next week-end
There was a problem hiding this comment.
I updated the PR with additional tests and fixed the lowering
This came up during an internship I was doing last year, I completely forgot about the exact setup and example. I'll try to recreate one during the week-end |
Here is an example: |