This is an archive of the discontinued LLVM Phabricator instance.

[mlir][tosa] Add tosa.table lowering to linalg.generic
ClosedPublic

Authored by rsuderman on Apr 1 2021, 11:36 AM.

Details

Summary

Table op lowering to linalg.generic for both i8 (behaves like a gather) and a
pair of gathers with a quantized interpolation.

Diff Detail

Event Timeline

rsuderman created this revision.Apr 1 2021, 11:36 AM
rsuderman requested review of this revision.Apr 1 2021, 11:36 AM
rsuderman updated this revision to Diff 334787.Apr 1 2021, 11:43 AM

Fixed error message.

rsuderman updated this revision to Diff 334788.Apr 1 2021, 11:43 AM

Removed superfluous comment.

rsuderman updated this revision to Diff 334856.Apr 1 2021, 4:47 PM

Added class comment for Table Lowering.

rsuderman updated this revision to Diff 335013.Apr 2 2021, 12:41 PM

Updated return statement.

hanchung requested changes to this revision.Apr 4 2021, 9:11 AM

Sorry for the late review...

mlir/lib/Conversion/TosaToLinalg/TosaToLinalg.cpp
1422–1424

This is not a template class, so we don't need template here.

1430–1432

Can we move this above *ElementTy declarations? It would look more straight-forward to me.

1432

I think probably say require inputTy to have static shape? Meaning it is a requirement for matching.

1436

[optional] I personally would use ArrayRef<Value>{}

1449

I would rather create a block manually than use nested builder. Because

  1. There are less indent.
  2. You are creating a complicated body.
  3. You can return a message when failing on creating the body.

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.

1450–1451

I think we can relax the condition to be "if all the element types are integer type and they have the same bitwidth?

1466

I feel this could be nestedLoc, nestedBuilder.getI32IntegerAttr(32768)

Same for below

1509

nit: there are two space right after +, delete one.

1527

drop punctuation at the send as these are supposed to be sentence fragments

1557–1559

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
864–865

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]]]
876

delete trailing blank lines

This revision now requires changes to proceed.Apr 4 2021, 9:11 AM
rsuderman updated this revision to Diff 335317.Apr 5 2021, 1:49 PM

Updated for round 1 review

rsuderman updated this revision to Diff 335318.Apr 5 2021, 1:51 PM
rsuderman marked 11 inline comments as done.

Missed comment

rsuderman marked an inline comment as done.Apr 5 2021, 1:51 PM
hanchung accepted this revision.Apr 6 2021, 1:12 AM
This revision is now accepted and ready to land.Apr 6 2021, 1:12 AM
rsuderman updated this revision to Diff 335617.Apr 6 2021, 12:03 PM

Fixed clang-tidy error

This revision was automatically updated to reflect the committed changes.