Added support for standalone mlir-conv #2110

ravil-mobile · 2023-08-23T10:18:51Z

Hi @pfultz2. I would like to add support for rocmlir standalone convolution ops. @manupak and I expect that it will beneficial on Navi cards.

It is my draft. Could you suggest something to be improved?

My first question is whether to add this feature to the existing fuse_mlir or make to it separate?

codecov · 2023-08-23T11:32:21Z

Codecov Report

Merging #2110 (d7e9763) into develop (d2486dc) will not change coverage.
The diff coverage is n/a.

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

@@           Coverage Diff            @@
##           develop    #2110   +/-   ##
========================================
  Coverage    91.43%   91.43%           
========================================
  Files          422      422           
  Lines        15771    15771           
========================================
  Hits         14420    14420           
  Misses        1351     1351

krzysz00 · 2023-08-23T14:41:35Z

src/targets/gpu/fuse_mlir.cpp

+MIGRAPHX_PRED_MATCHER(is_supported_arch, instruction_ref)
+{
+    // TODO(ravil): debug
+    static std::unordered_set<std::string> supported_consumer_archs{


This list isn't right.

Also, I'm pretty sure we're only meant to offload on gfx110x

This list isn't right.

You are correct. I just used it to test the concept on MI200 because I am still waiting to get access to a Navi machine.

From what I understood, gfx11* or gfx110* denote Navi3X GPUs. Maybe we can use a regex to capture current and future Navi cards?

Yi just updated me a few hours ago that gfx1100 for Navi31, gfx1101 for Navi32

krzysz00 · 2023-08-23T14:44:10Z

src/targets/gpu/fuse_mlir.cpp

+    void apply(module_pass_manager& mpm, const match::matcher_result& r) const
+    {
+        auto conv_based_op = r.result;
+        // Only fuse with fp32/fp16


Will we need this restriction?

Also, comment wrong

krzysz00 · 2023-08-23T14:45:04Z

src/targets/gpu/target.cpp

@@ -143,6 +144,7 @@ std::vector<pass> target::get_passes(migraphx::context& gctx, const compile_opti
 #endif
        dead_code_elimination{},
        enable_pass(mlir_enabled(), fuse_mlir{&ctx}),
+        enable_pass(mlir_enabled(), standalone_mlir{&ctx}),


Why's this a separate pass instead of being a rewrite within fuse_mlir() (which could be renamed to something like mlir_offload())?

What do folks think here?

It is a good question. @manupak and I was discussing this today - i.e., whether to add use one pass or two ones. My argument was that adding standalone conv to fuse_mlir was ambiguous. @manupak suggested that we could rename fuse_mlir as you mentioned.

By and large, yes. We can rename fuse_mlir to mlir_offload which sounds more generic.

pfultz2 · 2023-08-23T14:50:37Z

src/targets/gpu/fuse_mlir.cpp

+void standalone_mlir::apply(module_pass_manager& mpm) const
+{
+#ifdef MIGRAPHX_MLIR
+    match::find_matches(mpm, find_mlir_standalone_convolution_op{});


This should be moved to fuse_mlir::appy.

Got you. So, the I will remove
https://github.com/ravil-mobile/AMDMIGraphX/blob/63bdf8f04ab416d20023c7ac2acbb26df31172f3/src/targets/gpu/include/migraphx/gpu/standalone_mlir.hpp#L1-L47

pfultz2 · 2023-08-23T14:54:29Z

src/targets/gpu/fuse_mlir.cpp

+#ifdef MIGRAPHX_MLIR
+
+namespace {
+MIGRAPHX_PRED_MATCHER(is_supported_arch, instruction_ref)


I dont think this needs to be a matcher as it is not reading anything from the graph. It can be a vanilla function and we can call it to conditionally call match::find_matches(mpm, find_mlir_standalone_convolution_op{}).

Thanks! It something that we were discussing with @manupak today and came to the same conclusion. I will rework it

pfultz2 · 2023-08-23T14:55:32Z

src/targets/gpu/fuse_mlir.cpp

+        "gfx900", "gfx906", "gfx908", "gfx1030", "gfx940"};
+
+    // static std::unordered_set<std::string> supported_consumer_archs{"gfx1030"};
+    const auto device_name = trim(split_string(get_device_name(), ':').front());


The context should be used to get the device name.

I can see. Let me give a try.

* renamed the corresponding struct * addressed suggestions of PR ROCm#2110

manupak · 2023-08-24T11:31:21Z

I have no major concerns - I ll let maintainers approve it though coz I had some input into the code here.

Only concern is this code is not tested at the minute.
https://github.com/ROCmSoftwarePlatform/AMDMIGraphX/blob/6f1c947f686ee7e7265d14c6f416f4b82eec8815/Jenkinsfile#L125-L134

@pfultz2 since we add the flag to force standalone convs; we can test (the integration issues) in cdna using that flag OR do ya'll have Navi nodes to test this proper ?

pfultz2 · 2023-08-24T11:52:15Z

src/targets/gpu/CMakeLists.txt

@@ -109,7 +109,7 @@ add_library(migraphx_gpu
    compiler.cpp
    device_name.cpp
    fuse_ck.cpp
-    fuse_mlir.cpp
+    mlir_offload.cpp


This shouldn't be renamed in this PR. fuse_mlir is consistent with the other fuse passes. So if we rename this like this than we should rename all the other fuse passes. However, such refactoring should go into a separate PR.

@pfultz2, should I rename 1) just file names or 2) file names + the corresponding structures?

At this moment, I just changed file names. Let me know if I should to adapt the stuct names as well

I should to adapt the stuct names as well

Yes, the class names should match the filenames.

* renamed the corresponding struct * addressed suggestions of PR ROCm#2110

pfultz2 · 2023-08-28T14:34:28Z

src/targets/gpu/include/migraphx/gpu/mlir_offload.hpp

@@ -35,15 +35,15 @@ namespace gpu {

 MIGRAPHX_GPU_EXPORT bool mlir_enabled();

-struct MIGRAPHX_GPU_EXPORT fuse_mlir
+struct MIGRAPHX_GPU_EXPORT mlir_offload


The class should be named fuse_mlir.

But then it sounds as if we only provide fused operation. However, this PR aims at additionally providing standalone convs.

https://github.com/ravil-mobile/AMDMIGraphX/blob/e714ed3b5ce7fb39e09d10a7de0c4f4b02c43bbd/src/targets/gpu/fuse_mlir.cpp#L365-L381

Moreover, we are going to add standalone GEMMs. Additionally, we would like to have an option to switch off fused ops based on the value of an env. variables. In this case, we would like to completely fall back to standalone GEMMs and CONVs taking them from rocMLIR; we think it may be help us during our performance analyses and monitoring.

By and large, the apply method is not supposed to be focused on fused operations only; it is supposed to be generic. That is the reason why @krzysz00 and I proposed to rename the struct mlir_offload.

But then it sounds as if we only provide fused operation.

For fuse_ck, we handle standalone gemms, for fuse_pointwise we handle standalone pointwise, and fuse_reduce we handle standalone reduce. So I dont see how it would be different here.

By and large, the apply method is not supposed to be focused on fused operations only; it is supposed to be generic.

Yea niether do fuse_ck, fuse_pointwise, and fuse_reduce focus on fused operations only.

That is the reason why @krzysz00 and I proposed to rename the struct mlir_offload.

Yes, but such renaming should go in a separate PR, because the other fuse passes need to renamed as well.

Yes, but such renaming should go in a separate PR, because the other fuse passes need to renamed as well.

Got your point! Ok, I will rework.

* renamed the corresponding struct * addressed suggestions of PR ROCm#2110

* renamed the corresponding struct * addressed suggestions of PR #2110

…#2142)

ravil-mobile requested a review from manupak August 23, 2023 10:19

ravil-mobile force-pushed the ravil/standalone-mlir branch 2 times, most recently from 7f14839 to 63bdf8f Compare August 23, 2023 10:42

ravil-mobile requested a review from pfultz2 August 23, 2023 13:01

krzysz00 reviewed Aug 23, 2023

View reviewed changes

pfultz2 reviewed Aug 23, 2023

View reviewed changes

ravil-mobile added a commit to ravil-mobile/AMDMIGraphX that referenced this pull request Aug 23, 2023

Renamed fuse_mlir.hpp/cpp to mlir_offload.hpp/cpp

50b1321

* renamed the corresponding struct * addressed suggestions of PR ROCm#2110

ravil-mobile added a commit to ravil-mobile/AMDMIGraphX that referenced this pull request Aug 23, 2023

Renamed fuse_mlir.hpp/cpp to mlir_offload.hpp/cpp

76fe008

* renamed the corresponding struct * addressed suggestions of PR ROCm#2110

ravil-mobile force-pushed the ravil/standalone-mlir branch from 50b1321 to 76fe008 Compare August 23, 2023 18:54

ravil-mobile requested review from pfultz2 and krzysz00 August 23, 2023 18:56

ravil-mobile marked this pull request as ready for review August 23, 2023 19:26

pfultz2 reviewed Aug 24, 2023

View reviewed changes

jerryyin requested a review from kahmed10 August 24, 2023 13:43

ravil-mobile added a commit to ravil-mobile/AMDMIGraphX that referenced this pull request Aug 28, 2023

Renamed fuse_mlir.hpp/cpp to mlir_offload.hpp/cpp

8047a30

* renamed the corresponding struct * addressed suggestions of PR ROCm#2110

ravil-mobile force-pushed the ravil/standalone-mlir branch from 161fc20 to 81aa217 Compare August 28, 2023 14:22

pfultz2 reviewed Aug 28, 2023

View reviewed changes

ravil-mobile and others added 5 commits August 30, 2023 14:19

Added support for standalone mlir-conv

bcf55b6

Renamed fuse_mlir.hpp/cpp to mlir_offload.hpp/cpp

43455f8

* renamed the corresponding struct * addressed suggestions of PR ROCm#2110

Added env. variable to force standalone convs

efa1ab8

Reverted names i.e., mlir_offload.cpp to fuse_mlir.cpp

8b6d492

Implemted a more flexible selection of mlir ops

6d38378

ravil-mobile force-pushed the ravil/standalone-mlir branch from e714ed3 to 4da4a3e Compare August 30, 2023 14:19

Reverted names i.e., mlir_offload.hpp to fuse_mlir.hpp

1276c98

ravil-mobile force-pushed the ravil/standalone-mlir branch from 4da4a3e to 1276c98 Compare August 30, 2023 14:24

ravil-mobile requested a review from pfultz2 August 30, 2023 14:24

ravil-mobile closed this Aug 31, 2023

ravil-mobile mentioned this pull request Aug 31, 2023

[Re-Opened] Added support for standalone mlir-conv (used to be #2110) #2142

Merged

ravil-mobile added a commit that referenced this pull request Aug 31, 2023

Renamed fuse_mlir.hpp/cpp to mlir_offload.hpp/cpp

26d00c3

* renamed the corresponding struct * addressed suggestions of PR #2110

ravil-mobile added a commit that referenced this pull request Sep 4, 2023

Renamed fuse_mlir.hpp/cpp to mlir_offload.hpp/cpp

483e0d7

* renamed the corresponding struct * addressed suggestions of PR #2110

ravil-mobile added a commit that referenced this pull request Sep 6, 2023

Renamed fuse_mlir.hpp/cpp to mlir_offload.hpp/cpp

4ef9e64

* renamed the corresponding struct * addressed suggestions of PR #2110

ravil-mobile added a commit that referenced this pull request Sep 8, 2023

Renamed fuse_mlir.hpp/cpp to mlir_offload.hpp/cpp

6a34b34

* renamed the corresponding struct * addressed suggestions of PR #2110

ravil-mobile added a commit that referenced this pull request Sep 11, 2023

Renamed fuse_mlir.hpp/cpp to mlir_offload.hpp/cpp

9c618e3

* renamed the corresponding struct * addressed suggestions of PR #2110

ravil-mobile added a commit that referenced this pull request Sep 11, 2023

Renamed fuse_mlir.hpp/cpp to mlir_offload.hpp/cpp

358f8b1

* renamed the corresponding struct * addressed suggestions of PR #2110

ravil-mobile added a commit that referenced this pull request Sep 11, 2023

Renamed fuse_mlir.hpp/cpp to mlir_offload.hpp/cpp

a073c22

* renamed the corresponding struct * addressed suggestions of PR #2110

causten pushed a commit that referenced this pull request Sep 11, 2023

[Re-Opened] Added support for standalone mlir-conv (used to be #2110) (…

ea97ce5

…#2142)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for standalone mlir-conv #2110

Added support for standalone mlir-conv #2110

ravil-mobile commented Aug 23, 2023 •

edited

Loading

codecov bot commented Aug 23, 2023 •

edited

Loading

krzysz00 Aug 23, 2023

ravil-mobile Aug 23, 2023

krzysz00 Aug 23, 2023

krzysz00 Aug 23, 2023

krzysz00 Aug 23, 2023

ravil-mobile Aug 23, 2023

pfultz2 Aug 23, 2023

ravil-mobile Aug 23, 2023

pfultz2 Aug 23, 2023

ravil-mobile Aug 23, 2023

pfultz2 Aug 23, 2023

ravil-mobile Aug 23, 2023

manupak commented Aug 24, 2023 •

edited

Loading

pfultz2 Aug 24, 2023

ravil-mobile Aug 24, 2023 •

edited

Loading

ravil-mobile Aug 24, 2023

pfultz2 Aug 28, 2023

pfultz2 Aug 28, 2023

ravil-mobile Aug 28, 2023 •

edited

Loading

pfultz2 Aug 29, 2023

ravil-mobile Aug 30, 2023

ravil-mobile Aug 30, 2023

Added support for standalone mlir-conv #2110

Added support for standalone mlir-conv #2110

Conversation

ravil-mobile commented Aug 23, 2023 • edited Loading

codecov bot commented Aug 23, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manupak commented Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

ravil-mobile Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravil-mobile Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravil-mobile commented Aug 23, 2023 •

edited

Loading

codecov bot commented Aug 23, 2023 •

edited

Loading

manupak commented Aug 24, 2023 •

edited

Loading

ravil-mobile Aug 24, 2023 •

edited

Loading

ravil-mobile Aug 28, 2023 •

edited

Loading