-
Notifications
You must be signed in to change notification settings - Fork 367
Better handling of constants in -profile-ir #3257
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: main
Are you sure you want to change the base?
Better handling of constants in -profile-ir #3257
Conversation
…-all-ops option (default off) Signed-off-by: Alexandre Eichenberger <[email protected]>
Can we unify "profileAllOp" and "SkipConstant" into one name? If more op may be affected by this option in future, keep profileAllOp. Otherwise use SkipProfileConstant. |
That is a good point. At this time, when we "profileAllOps", I explicitly add profiling for the constant AND disable removing of empty profiling (namely where was an operation that was profiled, but that operation vanished). When we do not "prodileAllOps", then I don't add profiling for the constant AND remove empty profiling. That is why I have 2 names. But maybe when I don't have the constant, there are not really any empty operations... I can check and let you know. thanks @chentong319 . |
Signed-off-by: Alexandre Eichenberger <[email protected]>
Signed-off-by: Alexandre Eichenberger <[email protected]>
Implemented the requested changes, now the option is |
Signed-off-by: Alexandre Eichenberger <[email protected]>
Signed-off-by: Alexandre Eichenberger <[email protected]>
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.
LGTM!
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.
LGTM!
if (enableConstantOpProfiling) { | ||
instrumentOps += ",krnl.global"; | ||
instrumentSignatures += ",krnl.global"; | ||
} |
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 about why we need to add krnl.global
here while this is for profiling onnx ops. I though there were no krnl.global
ops at this IR level.
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.
Let me double check
Right now, when we do profiling of the IR, we get some constants (some are included, some are removed).
With this PR, the default with
-profile-ir
is to have as few constants in the runtime log as possible.But if the new option
-profile-all-ops
is set (default off), then the profiling attempts to profile all constants, as well as the ops that may have been removed (by optimizations).This allow us to get an idea of the memory pressure due to constants, for example by compiling with
-profile-ir=ZHigh -profile-all-ops
and log the output to a file.Then this command will show all of the constants in the model
where it lists all of the different shapes, and indicates how many of each of the shapes are present.