search

tenstorrent / tt-mlir

Tenstorrent MLIR compiler
https://tenstorrent.github.io/tt-mlir/
Apache License 2.0
63 stars 8 forks source link

Handle non-tile tensor inputs to binary ttnn ops #665

Open svuckovicTT opened 1 month ago

svuckovicTT commented 1 month ago

TTNN binary ops expect input tensors in ttnn::Layout::Tile format, and will assert otherwise.

svuckovicTT commented 1 month ago

We should probably use operand constraints API, so that TTNN dialect forces TTIR to use tiles for binary eltwise ops.

LPanosTT commented 3 weeks ago

The forcing to tile layout in TTIRToTTNN.cpp is blocking certain reshapes that are required for maxpool2d in resent. Also why is it that data must be in tile layout to perform an eltwise binary op? Any chance this could be changed in metal?

svuckovicTT commented 3 weeks ago

Also why is it that data must be in tile layout to perform an eltwise binary op? Any chance this could be changed in metal?

That's just the way the kernels are implemented. @mbezuljTT is working on modeling these constraints.

The forcing to tile layout in TTIRToTTNN.cpp is blocking certain reshapes that are required for maxpool2d in resent.

Can you show me where this happens? We should just read the layout from the tensors, not overwrite. In case the TTIR itself has the tile layout set for maxpool, we can add the constraint for that in the IR itself. If we're messing with something during dialect conversion itself, I can take a look tomorrow.

LPanosTT commented 3 weeks ago

Can you show me where this happens? https://github.com/tenstorrent/tt-mlir/blob/5e48f3ab147284116a0ca4ec1fbd30483155b284/lib/Conversion/TTIRToTTNN/TTIRToTTNN.cpp#L124

@svuckovicTT As you can see the layout that is determined by the if-else block above this line is overwritten with tile layout

svuckovicTT commented 3 weeks ago

Thanks @LPanosTT, let me look into it tomorrow, should be an easy fix, just need to make sure there's no fallout to other ops.

LPanosTT commented 3 weeks ago

just need to make sure there's no fallout to other ops.

@svuckovicTT Actually, I tried commending this out and it does cause eltwise binary ops to fail since they do not get formatted into tile layout :(

mbezuljTT commented 2 weeks ago

I'm sorry for late reply, I have a question - why dont we force all ops to Tile layout?

More specifically what are ops we know that work with scalar/row_major input?

is maxpool2d an outlier?

Bonus - what Buda ops were supporting row_major inputs? could someone help me identify gap between Buda/Forge on this?

mbezuljTT commented 2 weeks ago

We are discussing this on the following issue: https://github.com/tenstorrent/tt-mlir/issues/272

svuckovicTT commented 4 days ago

I'm working on a change to undo the tile hack... Just left to update tests, I'll probably have it up on a PR tomorrow:

https://github.com/tenstorrent/tt-mlir/tree/svuckovic/fix-665-2

svuckovicTT commented 4 days ago

https://github.com/tenstorrent/tt-mlir/pull/863