Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for standalone dot operations with mlir #2169

Merged
merged 8 commits into from
Sep 16, 2023

Conversation

ravil-mobile
Copy link
Contributor

@ravil-mobile ravil-mobile commented Sep 12, 2023

The PR enables the code generation of standalone dot operations with MLIR.

@manupak, could you, please, have a look at this PR. If everything is ok I will ask Paul to make a review.

Hi @pfultz2, could you, please, make a review of this PR? It extends PR #2142 by adding the option to generate standalone dot operations.

@ravil-mobile ravil-mobile marked this pull request as ready for review September 12, 2023 10:03
Copy link
Contributor

@manupak manupak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There would be module name collisions. I have suggested one way to fix it.

src/targets/gpu/fuse_mlir.cpp Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

Merging #2169 (f47abbb) into develop (205306a) will not change coverage.
The diff coverage is n/a.

❗ Current head f47abbb differs from pull request most recent head c5c974a. Consider uploading reports for the commit c5c974a to get more accurate results

@@           Coverage Diff            @@
##           develop    #2169   +/-   ##
========================================
  Coverage    91.48%   91.48%           
========================================
  Files          426      426           
  Lines        15927    15927           
========================================
  Hits         14571    14571           
  Misses        1356     1356           

@ravil-mobile
Copy link
Contributor Author

There would be module name collisions. I have suggested one way to fix it.

Resolved

Copy link
Contributor

@manupak manupak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@migraphx-bot
Copy link
Collaborator

migraphx-bot commented Sep 12, 2023

Test Batch Rate new
c5c974
Rate old
74ba96
Diff Compare
torchvision-resnet50 64 2,282.89 2,279.58 0.15%
torchvision-resnet50_fp16 64 5,359.95 5,356.99 0.06%
torchvision-densenet121 32 1,816.65 1,830.89 -0.78%
torchvision-densenet121_fp16 32 3,393.58 3,385.93 0.23%
torchvision-inceptionv3 32 1,340.17 1,343.88 -0.28%
torchvision-inceptionv3_fp16 32 2,591.51 2,594.96 -0.13%
cadene-inceptionv4 16 678.64 680.41 -0.26%
cadene-resnext64x4 16 589.71 591.01 -0.22%
slim-mobilenet 64 7,210.29 7,222.64 -0.17%
slim-nasnetalarge 64 236.86 236.81 0.02%
slim-resnet50v2 64 2,526.81 2,527.30 -0.02%
bert-mrpc-onnx 8 720.80 721.29 -0.07%
bert-mrpc-tf 1 389.43 389.65 -0.06%
pytorch-examples-wlang-gru 1 304.63 306.53 -0.62%
pytorch-examples-wlang-lstm 1 313.53 314.52 -0.32%
torchvision-resnet50_1 1 559.38 559.67 -0.05%
torchvision-inceptionv3_1 1 306.51 303.71 0.92%
cadene-dpn92_1 1 353.73 354.49 -0.21%
cadene-resnext101_1 1 220.39 220.64 -0.11%
slim-vgg16_1 1 224.27 224.76 -0.22%
slim-mobilenet_1 1 1,489.84 1,461.10 1.97%
slim-inceptionv4_1 1 222.29 222.33 -0.02%
onnx-taau-downsample 1 322.50 322.59 -0.03%
dlrm-criteoterabyte 1 21.70 21.70 -0.01%
dlrm-criteoterabyte_fp16 1 40.59 40.63 -0.11%
agentmodel 1 5,846.28 5,843.44 0.05%
unet_fp16 2 55.10 55.10 -0.01%

This build is OK for merge ✅

@migraphx-bot
Copy link
Collaborator

migraphx-bot commented Sep 12, 2023


    :white_check_mark:bert-mrpc-onnx: PASSED: MIGraphX meets tolerance

    :white_check_mark:bert-mrpc-tf: PASSED: MIGraphX meets tolerance

    :white_check_mark:pytorch-examples-wlang-gru: PASSED: MIGraphX meets tolerance

    :white_check_mark:pytorch-examples-wlang-lstm: PASSED: MIGraphX meets tolerance

    :white_check_mark:torchvision-resnet50_1: PASSED: MIGraphX meets tolerance

🔴torchvision-inceptionv3_1: FAILED: MIGraphX is not within tolerance - check verbose output


🔴cadene-dpn92_1: FAILED: MIGraphX is not within tolerance - check verbose output


    :white_check_mark:cadene-resnext101_1: PASSED: MIGraphX meets tolerance

    :white_check_mark:slim-vgg16_1: PASSED: MIGraphX meets tolerance

    :white_check_mark:slim-mobilenet_1: PASSED: MIGraphX meets tolerance

🔴slim-inceptionv4_1: FAILED: MIGraphX is not within tolerance - check verbose output


    :white_check_mark:dlrm-criteoterabyte: PASSED: MIGraphX meets tolerance

    :white_check_mark:agentmodel: PASSED: MIGraphX meets tolerance

    :white_check_mark:unet: PASSED: MIGraphX meets tolerance

return false;
}
return is_requested("dot");
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just make a is_standalone_op_enabled that takes the op name as a parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pfultz2. Maybe, instead of is_standalone_op_enabled, we can generalize it for all ops and call it, for example, is_op_enabled?

What do you think about the following code?

bool is_op_enabled(std::string_view op_name, context* ctx)
{
    if(is_self_decide())
    {
        if(op_name == "fused")
        {
            return true;
        }
        else if(op_name == "convolution")
        {
            if(ctx == nullptr)
            {
                return false;
            }
            else
            {
                const auto& device = ctx->get_current_device();
                const std::string navi_family{"gfx110"};
                return starts_with(device.get_gfx_name(), navi_family);
            }
        }
        else
        {
            return false;
        }
    }
    return is_requested(op_name);
}

} // namespace

#endif // MIGRAPHX_MLIR

void fuse_mlir::apply(module_pass_manager& mpm) const
{
#ifdef MIGRAPHX_MLIR
    if(is_op_enabled("fused", this->ctx))
    {
        match::find_matches(mpm, find_mlir_fused_ops{});
    }

    if(is_op_enabled("convolution", this->ctx))
    {
        match::find_matches(mpm, find_mlir_standalone_convolution_op{});
    }

    if(is_op_enabled("dot", this->ctx))
    {
        match::find_matches(mpm, find_mlir_standalone_dot_op{});
    }
#else
    (void)mpm;
#endif
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea thats a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Done.

@causten
Copy link
Collaborator

causten commented Sep 15, 2023

@ravil-mobile don't merge develop in to this PR anymore, I have it in the merge pipeline and additional merges overload CI.

@causten causten merged commit 0064036 into develop Sep 16, 2023
12 checks passed
@causten causten deleted the ravil/standalone-mlir branch September 16, 2023 11:13
@ravil-mobile
Copy link
Contributor Author

@ravil-mobile don't merge develop in to this PR anymore, I have it in the merge pipeline and additional merges overload CI.

Ok, I didn't realize that there was a dedicated merge pipeline

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants