-
Notifications
You must be signed in to change notification settings - Fork 49
Fix usage of host-pipeline=highlevel
#2020
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
base: develop
Are you sure you want to change the base?
Conversation
isHighLevel, kernelPipelineSet); | ||
kernelPipelineSet); | ||
// Run host high-level pipeline if specified | ||
if (hostPipelineSet.contains("highlevel")) |
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.
shouldn't this be kernelPipelineSet.contains("highlevel")? are we running highlevel for the kernel here?
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.
if that's the case, maybe we should change the name of the function as well: runHostHighLevelPipeline -> runHighLevelPipeline ?
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.
runHostHighLevelPipeline
is only running highlevel for the host path, so we want to keep it as is. If kernelPipelineSet.contains("highlevel")
then this will trigger that specific pipeline to be run in runKernelPipeline
. This fixes previous behavior where hostPipelineSet.contains("highlevel")
would then cause for runKernelPipeline
run (i.e., the weird behavior where setting host-pipeline=highlevel
would run both the CPU and GPU paths).
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 understand now. However, the logic is hard to follow. IMO it'd be better if we handle them separately instead of if(!kernelPipelineSet.empty() || hostPipelineSet.contains("highlevel")) in line 305. But, that's out of the scope of this PR.
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.
Looking good, but I think it would benefit a lot from:
- Throwing errors in the
if (!func->hasAttr("kernel"))
case - Splitting the pipelines execution in 2:
runHostPipeline
andrunKernelPipeline
auto func = getOperation(); | ||
if (!func->hasAttr("kernel")) { | ||
return; | ||
llvm::report_fatal_error("func op does not have the kernel attribute"); |
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.
This is great! But I think this PR is missing more of these.
In my opinion this PR must have a commit that do the same for all occurrences in grep -R 'hasAttr("kernel"))' mlir/lib/
.
The pattern:
if(thisShouldNeverHappen)
return;
is evil. In my opinion we should avoid this pattern at all costs.
@@ -134,8 +131,33 @@ parsePipeline(StringRef pipeline, llvm::SmallDenseSet<StringRef> &pipelineSet, | |||
return success(); | |||
} | |||
|
|||
static LogicalResult runHostHighLevelPipeline(ModuleOp mod) { |
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 think this is a step in the right direction. And I know it's quite some work but I think it would be good if we had two functions:
runHostPipeline
(for the host only)runKernelPipeline
(for the gpu only)
If you like this it could go into another PR, or maybe here, not sure
return success(); | ||
} | ||
|
||
static LogicalResult runHostHighLevelPipeline(ModuleOp mod) { |
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.
nit: Use either m
or module
instead of mod
(I've seen the first 2 quite often, but never mod
).
|
||
static LogicalResult | ||
runKernelPipeline(StringRef arch, ModuleOp kmod, bool isHighLevel, | ||
runKernelPipeline(StringRef arch, ModuleOp kmod, |
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.
nit: Same here, I'd use m
or module
instead of kmod
bool hasKernels = false; | ||
// Find kernel module, defaults to top module | ||
if (!kernelPipelineSet.empty() || isHighLevel) { | ||
if (!kernelPipelineSet.empty() || hostPipelineSet.contains("highlevel")) { |
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've been staring at this for a while and I still don't know what this if is checking for exactly. Is it checking if kernel pipeline was specified or if host pipeline was specified with highlevel
? If this is the case I find it weird that we have a lot of code for that specific case.
I know it's not strictly related to the PR but can you think of a concise name and use a boolean variable for this specific case? It would help readability.
@@ -1,4 +1,4 @@ | |||
// RUN: rocmlir-driver -kernel-pipeline migraphx,highlevel %s | rocmlir-gen -ph -print-results -rand none -fut test - | \ | |||
// RUN: rocmlir-driver --host-pipeline=migraphx,highlevel %s | rocmlir-gen -ph -print-results -rand none -fut test - | \ |
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.
Just curious: Why =
instead of a space? Is it different?
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.
There's no difference. We use a combination of both methods all over our LIT tests
Motivation
The current usage of
rocmlir-driver
is unintuitive and may lead to frustrating debug sessions. When running the following command:rocmlir-driver -kernel-pipeline=migraphx -host-pipeline=migraphx,highlevel
, callinghighlevel
for the host pipeline also runs the functions marked withkernel
(i.e., it is also doing the job of-kernel-pipeline=highlevel
). This is incredibly unintuitive, and breaks from the behavior thatmigraphx
, and all the other pipelines have.This PR fixes the following issue: https://github.com/ROCm/rocMLIR-internal/issues/1983
Technical Details
This PR implements the following changes:
TosaToRockPass
so that whenkernel-pipeline=highlevel
is run on a function without thekernel
attribute, it will error out instead of silently doing nothing (we can safely do this now that the kernel pipeline and host pipeline are separate)rocmlir-driver
to separatekernel-pipeline=highlevel
andhost-pipeline=highlevel
runKernelPipeline
. I've pulled out the necessary logic into a separate functionrunHostHighLevelPipeline
cpu-only
flagrocmlir-driver
Test Plan
Test Result
Submission Checklist