This is an archive of the discontinued LLVM Phabricator instance.

[mlir][tosa] Improve lowering support for tosa.concat
ClosedPublic

Authored by sabauma on May 30 2023, 6:23 AM.

Details

Summary

The existing lowering for tosa.concat fails in some instances when the
output shape contains more information the input shapes. The result is
an illegal tensor.empty operation.

This change bases the output shape on the original tosa.concat
operation, while querying the input tensor shapes to build the slicing
operations.

Diff Detail

Event Timeline

sabauma created this revision.May 30 2023, 6:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2023, 6:23 AM
sabauma edited the summary of this revision. (Show Details)May 30 2023, 6:29 AM
sabauma added reviewers: rsuderman, eric-k256, jpienaar.
sabauma published this revision for review.May 30 2023, 7:18 AM
rsuderman requested changes to this revision.Jun 2 2023, 12:17 PM
rsuderman added inline comments.
mlir/lib/Conversion/TosaToTensor/TosaToTensor.cpp
374

Why do you make this a one line helper. This is simple enough it should just be inlined.

mlir/test/Conversion/TosaToTensor/tosa-to-tensor.mlir
244

This reference %dim but no definition of where it came from. I believe this should be the summation of the two correct?

267

We should see CHECK-DAG in most of these tests. Your current implementation enforces order which doesn't work for constants very well.

274

There are no cases that should computing the resulting dynamic length on the axis dimension. Considering most of your changes focus on supporting concatenation across dynamic dims I would expect to see these behavior tested more directly.

291

This is not a good test. Assuming dynamic ? values and static values being safe to concat is potentially unsafe (in the cases they don't match). We allow these mismatches as an intermediate but it produces possibly illegal behavior.

This revision now requires changes to proceed.Jun 2 2023, 12:17 PM
sabauma updated this revision to Diff 527967.Jun 2 2023, 1:59 PM
sabauma edited the summary of this revision. (Show Details)

Address rsuderman's comments

  1. Replace uses of CHECK with CHECK-DAG
  2. Fix some test cases to properly match on captured patterns
  3. Fix an issue in the computation of the output size along the axis dimension
sabauma updated this revision to Diff 527985.Jun 2 2023, 2:34 PM
sabauma marked 2 inline comments as done.

Lock down computation of the total size of the output tensor

sabauma marked 6 inline comments as not done.Jun 2 2023, 2:38 PM
sabauma added inline comments.
mlir/test/Conversion/TosaToTensor/tosa-to-tensor.mlir
244

Fixed this and a few other similar cases.

267

I converted all the tests I modified to use CHECK-DAG rather than CHECK.

274

There are no cases that should computing the resulting dynamic length on the axis dimension.

The lowering does produce a computation of the total size along the concatenation axis. I've updated the test to lock down that computation. I originally omitted it as the actual value of the computation is not used.

Considering most of your changes focus on supporting concatenation across dynamic dims I would expect to see these behavior tested more directly.

The specific issue I am trying to address is when the output dimensions of the tosa.concat are more specified (fewer dynamic dimensions) than the input dimensions. The old lowering would produce a tensor.empty operation like this:

tensor.empty(%d0, %d1) : tensor<5x1x10xf32>

Which fails to verify, as there are more arguments than dynamic dimensions on the output tensor.

I don't mind adding more if you feel that is warranted, but the other tests I updated are also testing concatenation operations with dynamic dimensions, so I opted for testing the specific case I'm aiming to fix.

291

I agree that it is odd to have more information about the result type of the concatentation than the input types.

Some of the other concat tests have similar issues with potential mismatches along dynamic dimensions, and I'm not really sure what I can do to fix that short of generating runtime logic to verify these constraints.

sabauma updated this revision to Diff 528924.Jun 6 2023, 10:17 AM

Convert additional concat test to use CHECK-DAG

sabauma marked an inline comment as done.Jun 6 2023, 3:21 PM
sabauma added inline comments.
mlir/lib/Conversion/TosaToTensor/TosaToTensor.cpp
374

I've eliminated the helper function in factor of direct use.

sabauma marked an inline comment as done.Jun 7 2023, 6:17 AM

@rsuderman Could you elaborate more on your last point in the review:

This is not a good test. Assuming dynamic ? values and static values being safe to concat is potentially unsafe (in the cases they don't match). We allow these mismatches as an intermediate but it produces possibly illegal behavior.

I am uncertain how to address this concern in an operator lowering. Some of the other concat test examples potentially have the same issue.

rsuderman added inline comments.Jun 12 2023, 10:00 AM
mlir/test/Conversion/TosaToTensor/tosa-to-tensor.mlir
291

You should have two versions of this test. One where the concatenation is along a ? dimension, and another where a non-concat dimension is ?. We want to see that the offsets involving ? are generated by computing using tensor.dim.

rsuderman added inline comments.Jun 12 2023, 10:15 AM
mlir/lib/Conversion/TosaToTensor/TosaToTensor.cpp
375

Either initially set this to 0 or compute within the loop. The axisOffsets entries should match 1-1 with eh operands as their expected offset.

410

Keep the arith::AddIOp here or generate axisOffsets so that correspond with each insertion. It is strange to see mutating the variable depending on the next iteration of the loop.

mlir/test/Conversion/TosaToTensor/tosa-to-tensor.mlir
232

Your last CHECK* statement always needs to be a CHECK not a CHECK-DAG so it knows to check the values as the root.

254

Ditto: last CHECK-DAG should be a CHECK

sabauma updated this revision to Diff 530891.Jun 13 2023, 6:41 AM

Address rsuderman's comments on test cases

sabauma marked 6 inline comments as done.Jun 14 2023, 6:39 AM

Addressed @rsuderman's feedback.

sabauma marked an inline comment as done.Jun 14 2023, 6:40 AM
rsuderman accepted this revision.Jun 15 2023, 11:22 AM
This revision is now accepted and ready to land.Jun 15 2023, 11:22 AM
This revision was automatically updated to reflect the committed changes.