-
Notifications
You must be signed in to change notification settings - Fork 31
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
Integrating alp with iree-llvm-sandbox #83
Conversation
0096c00
to
495f7b2
Compare
@@ -32,6 +32,15 @@ set(MLIR_MAIN_SRC_DIR ${LLVM_MAIN_SRC_DIR}/../mlir) | |||
set(MLIR_INCLUDE_DIR ${LLVM_MAIN_SRC_DIR}/../mlir/include) | |||
set(MLIR_TABLEGEN_OUTPUT_DIR ${CMAKE_BINARY_DIR}/tools/mlir/include) | |||
|
|||
# Disable experimental alp by default | |||
set(SANDBOX_ENABLE_ALP OFF) | |||
if (SANDBOX_ENABLE_ALP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with this level of integration, I agree for projects that want it it is good to be buildable and integrated from the get go.
The fact that it is optional and cannot break the CI SGTM!
using namespace mlir; | ||
|
||
namespace{ | ||
struct ModuloSchedulingPass : public ModuloSchedulingPassBase<ModuloSchedulingPass> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not reviewing the pass itself for now, this would need a bunch of .mlir tests to qualify as "in reviewable state".
SGTM, since everything is optional I don't see an issue.
Sounds good for a first commit and to get started. I still think in the future the proper efforts should be made to either integrate with the harness or to come up with something better we can all reuse; I added comments where appropriate.
I gave you review comments, no need to address everything now but please open the proper issues (prefixed with |
Note I create and landed the alp subdir in an effort to try and give you rights on it specifically. It will take me a little time as I have a bunch of stuff in my stack. |
Thanks Nicolas for this first round of comments. I started to reply to some. Few overall replies:
One question I didn't ask yet. Do you have any performance result for x86? Do they look promising? |
Re. Still, it looks like you have a string-stichy and inflexible bandaid that limits you in ways you may not realize (what you have is very close to what the sandbox started with, we invested into making the QoL better for these reasons). You have to decide Re x86, the various benchmarks run at a high fraction of peak on my AVX512 machine. I have previously dumped some perf results here:
The matmul cases has mixes of divisible and non-divisble sizes; I get similar perf with Lastly, I did some moderate amount of manual searching to get to these perf points, scaling up search (esp. for the kernel) and rooting out the type of issue you mention re outlining will be important for us too. Hope this helps! |
could you please rebase/fix CI/land? |
5c535ca
to
76945be
Compare
76945be
to
ec5f72f
Compare
I tried to rebase and fix few things. Let's see if CI is happy. |
Alright, CI is happy, but I am not authorized to merge |
This is the first integration for our project in
alp/experimental
. I tried to make the minimal number of changes outside of this folder, but:mlir-proto-opt
experimental/alp
folder from the rootCMakeLists.txt
(although everything is disabled by default)All of this is super-early stage and it is very buggy and not tested. However, I wanted to show earlier than later how we would like to integrate with this, and what were the main ideas. In particular:
mlir-proto-opt
, but not reusing the Pyhton harness you have)README.md
to explain how to use it.Thank you so much,
Giuseppe