Page MenuHomePhabricator

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

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



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...


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


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


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


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


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.


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


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

Same for below


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


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


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...


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

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.