search

tenstorrent / tt-metal

:metal: TT-NN operator library, and TT-Metalium low level kernel programming model.
Apache License 2.0
428 stars 57 forks source link

Add output tensors parameter #5044

Closed eyonland closed 4 months ago

eyonland commented 8 months ago

Changing the Op from...

struct MorehOp {
    void validate(const std::vector<Tensor> &input_tensors) const;
    std::vector<Shape> compute_output_shapes(const std::vector<Tensor> &input_tensors) const;
    std::vector<Tensor> create_output_tensors(const std::vector<Tensor> &input_tensors) const;
    operation::ProgramWithCallbacks create_program(
        const std::vector<Tensor> &input_tensors, std::vector<Tensor> &output_tensors) const;
};

To...

struct MorehOp {
    void validate(const std::vector<Tensor> &input_tensors, std::vector<Tensor> &output_tensors) const;
    std::vector<Shape> compute_output_shapes(const std::vector<Tensor> &input_tensors) const;
    std::vector<Tensor> create_output_tensors(const std::vector<Tensor> &input_tensors, std::vector<Tensor> &output_tensors) const;
    operation::ProgramWithCallbacks create_program(
        const std::vector<Tensor> &input_tensors, std::vector<Tensor> &output_tensors) const;
};

Please see the following document for requirements on the deliverables. Requirements for Optional Output Tensors

eyonland commented 8 months ago

MNIST

GPT2

jliangTT commented 8 months ago

@razorback3 - FYI the feature for Output tensor as argument of the OP API

muthutt commented 8 months ago

Issue #5044

jliangTT commented 8 months ago

@muthutt , will you cover this function in #5163 as part of the migration? Are you still on-track for 02/09 for the original ask?

muthutt commented 8 months ago

yes

On Wed, Feb 7, 2024 at 12:49 PM jliangTT @.***> wrote:

@muthutt https://github.com/muthutt , will you cover this function in

5163 https://github.com/tenstorrent-metal/tt-metal/issues/5163 as part

of the migration? Are you still on-track?

— Reply to this email directly, view it on GitHub https://github.com/tenstorrent-metal/tt-metal/issues/5044#issuecomment-1932845671, or unsubscribe https://github.com/notifications/unsubscribe-auth/BAGOCNFEMGP2NDSP6FM2VHDYSPSD7AVCNFSM6AAAAABCTTNDECVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZSHA2DKNRXGE . You are receiving this because you were mentioned.Message ID: @.***>

jliangTT commented 6 months ago

@umadevimcw , as discussed. can we provide ETA for operators (should including backward) for MNIST-need op seperately from the GPT2-needed op. (see the grouping here - https://github.com/tenstorrent-metal/tt-metal/issues/5044#issuecomment-1919763818)

jliangTT commented 6 months ago

As discussed in the slack - @razorback3 has agreed to split the roll out according to the ownership as follows (this is the same as spreadsheet)

Legend: M for Moreh | T for Tenstorrent

MNIST M - Linear - #5044: Add output tensors support for linear op #6259 M- LogSoftmax M - NLLLoss M - SGD T - Argmax T - ElemwiseBinary/ELEMWISE_BINARY_EQ T - Typecast M - Sum - Add optional output tensor for Moreh_sum #6591 GPT2 M - BatchedMatmul - Add optional tensor support for bmm #6541 T - ElemwiseBinary/ELEMWISE_BINARY_ADD T - ElemwiseBinary/ELEMWISE_BINARY_ADDALPHA T - ElemwiseBinary/ELEMWISE_BINARY_MUL T - ElemwiseUnary/ELEMWISE_UNARY_ADD T - ElemwiseUnary/ELEMWISE_UNARY_ASSIGN T - ElemwiseUnary/ELEMWISE_UNARY_DIV T - ElemwiseUnary/ELEMWISE_UNARY_EXP T - ElemwiseUnary/ELEMWISE_UNARY_FILL T - ElemwiseUnary/ELEMWISE_UNARY_FILLZERO T - ElemwiseUnary/ELEMWISE_UNARY_MUL T - ElemwiseUnary/ELEMWISE_UNARY_POW T - ElemwiseUnary/ELEMWISE_UNARY_SQRT T - ElemwiseUnary/ELEMWISE_UNARY_TANH M - Layernorm M - Linear M - Matmul - Add output tensors support for matmul #6588 (moreh) M - Softmax M - AdamW M - Arange M - ClipGradNorm T - ElemwiseTernary/ELEMWISE_TERNARY_ADDCDIV T -ElemwiseTernary/ELEMWISE_TERNARY_ADDCMUL T - Embedding M - GenerateRandomBitMask M - NLLLoss M - NLLLossMeanPre M - NLLLossUnreduced T - Repeat M - Sum T -Typecast T - Where M - DropoutScaling M - Groupnorm M - MaskedFill T - Prod M - Tril

razorback3 commented 6 months ago

@jliangTT As we split the ownership, is it okay to ignore issues/pull-requests related to this issue and raised by Metal team on Moreh ops? For example, PR #6259.

jliangTT commented 6 months ago

yes, we should abandon those PRs. @eyonland @umadevimcw , are you in agreement with that?

eyonland commented 6 months ago

@razorback3 and @jliangTT, we should close these pull requests that have moreh ops on them.

If I need to make an infra change, to demonstrate a proposal, I'll make a new PR and explicitly state that.

jliangTT commented 6 months ago

@eyonland , what is the latest status for this?

KalaivaniMCW commented 6 months ago

@eyonland Could you take a look at this commit - (changes for adding optional output tensors to binary ops.)

jliangTT commented 5 months ago

This is currently blocked from TTNN refactor items. Our review with @eyonland is we need to sync up internally to see when we can execute on this.

davorchap commented 4 months ago

The highest priority list:

Linear
Softmax
NLLLoss
SGD
Argmax
ElemwiseBinary/ELEMWISE_BINARY_EQ
Typecast
Sum
KalaivaniMCW commented 4 months ago

Hi @eyonland @arakhmati I have added optional output tensor to binary ops ( includes Binary EQ, ADD MUL) , with operation::launch_op, operation::run and removed autoformat operations. I have made fixes for those ops that were dependent on autoformat functionality. Except for prod_bw (some issues with permute and unpad) all the other ops support the changes . CI : https://github.com/tenstorrent/tt-metal/actions/runs/9112666143/ draft PR : https://github.com/tenstorrent/tt-metal/pull/8394

razorback3 commented 4 months ago

@dongjin-na Would you check the above PR?

dongjin-na commented 4 months ago

I reviewed the 8394 PR and ran test_eltwise_binary_optional_output.py. Through testing, I confirmed that this PR supports the optional output tensor for eltwise_binary_op. Although the test items were EQ, MUL, and ADD, I expect other ops will also function well.

Here are a few comments regarding this:

KalaivaniMCW commented 4 months ago

@seunghwan100 @davorchap @eyonland @arakhmati

Hi @dongjin-na,

For composite and backward ops :

davorchap commented 4 months ago

MNIST

These need optional output tensor and also have additional issues:

KalaivaniMCW commented 4 months ago

Added optional output tensor for eltwise binary ops (fwd)

davorchap commented 4 months ago

what's the status of these for optional output tensor @umadevimcw

MNIST

Linear - https://github.com/tenstorrent/tt-metal/pull/6259 LogSoftmax NLLLoss SGD Argmax ElemwiseBinary/ELEMWISE_BINARY_EQ - https://github.com/tenstorrent/tt-metal/pull/8394 Typecast Sum - https://github.com/tenstorrent/tt-metal/issues/6591