-
Notifications
You must be signed in to change notification settings - Fork 326
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
Zmodel using the device placement approach #2534
Zmodel using the device placement approach #2534
Conversation
… with tags Signed-off-by: Alexandre Eichenberger <[email protected]>
Signed-off-by: Alexandre Eichenberger <[email protected]>
Signed-off-by: Alexandre Eichenberger <[email protected]>
Signed-off-by: Alexandre Eichenberger <[email protected]>
Signed-off-by: Alexandre Eichenberger <[email protected]>
Signed-off-by: Alexandre Eichenberger <[email protected]>
return WalkResult::advance(); | ||
// Now we have an operation that can work on the NNPA, check if its | ||
// beneficial | ||
if (useZHighCostModel && !isOpFasterOnNNPA(op, &dimAnalysis)) { |
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.
@tungld Please let me know if that is how you envisioned this to be used. I placed it there because to the extend possible, I only want to invoke the isOpFasterOnNNPA
for operations that I know are candidates... otherwise there might be a lot of ops that are irrelevant to being with. Open to suggestion if you had something else in mind.
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.
@AlexandreEichenberger I plan to do this earlier, at Line 74. The code here will respect what you add by the cost model (e.g. device = cpu
). Something like this at Line74.
if (useZHighCostModel) {
module.walk([&](Operation *op) {
if (!isOpFasterOnNNPA(op, &dimAnalysis))
op->setAttr(DEVICE_ATTRIBUTE, StringAttr::get(context, CPU_DEVICE));
});
}
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.
The legality check is aware of device=cpu
in an operation if it is set, so the compiler will assign NNPA for the other ops only.
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.
It's possible, but it will see all of the onnx ops, as opposed to only the ops that were deemed to be target for NNPA.
So it should run faster (seen fewer ops) and also report on only ops that qualify. That helps for printing debugging (only focusing on the important ops). So do you think its worth it to move the code before?
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.
Yes, it is faster here. Actually, the code here is to annotate ops based the result of analysis of rewriting patterns, so it's a bit confusing if we add the cost model here. So, we will annotate the ops using the cost model first, and the rewrite patterns are internally respect device
attribute annotated by the cost model.
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.
@AlexandreEichenberger fyi, I am creating a PR to set device by using a json file: #2536. It works now but still needs to add some lit test.
src/Dialect/ONNX/Rewrite.cpp
Outdated
@@ -914,6 +914,11 @@ class PowToMulRewritePattern : public OpRewritePattern<ONNXPowOp> { | |||
private: | |||
// Check if a Pow can be simply rewritten as a sequence of multiply ops. | |||
bool CanExpandPowOpToMul(ONNXPowOp op, int64_t &powVal) const { | |||
#if 1 // hi alex |
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.
Will remove, there is still something I need to investigate.
Signed-off-by: Alexandre Eichenberger <[email protected]>
Signed-off-by: Alexandre Eichenberger <[email protected]>
@tungld I have a switch
Lots of ops that don't qualify. I can add all of the ops that might go to the NNPA and leave them without preference, and then force all the ones I know don't work and force them to CPU, but then we have a redundant system. The perf model then decides which one are forced to CPU because we don't support it, and if it get's it wrong, then the later legalize never sees them because they are already forced to CPU... Or I leave them undecided, and then the legalize also see all of the ops. Let me know if you still feel it's better to have it before. |
Signed-off-by: Alexandre Eichenberger <[email protected]>
Oh, now I understand that you would like to apply the cost model only for the NNPA operations, and put them back to CPU if beneficial. I thought that the cost model would finally analyze the whole model and relationship between the ops to give better decision, so I proposed to do it separately. Just a note for me for the future: I am a bit concern about the inconsistency. The current walk is to annotate ops based the result of applying rewriting patterns. And, the rewriting patterns are able to detect
Yes, legalization can see all the ops. |
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. Thanks for the great contribution!
return WalkResult::advance(); | ||
}); | ||
} | ||
#endif |
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 the situation now. You can remove this.
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
//===----------------- Auto-Generated, do not change ---------------------===// |
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.
Could you give additional comments on how to generate this? e.g. scripts, commands
That is my next step, in that step, I also will need to know which op is legal for the NNPA. We can chat how to do that best. |
Signed-off-by: Alexandre Eichenberger <[email protected]>
Signed-off-by: Alexandre Eichenberger <[email protected]>
Jenkins Linux s390x Build #12857 [push] Zmodel using the device ... started at 12:30 |
Jenkins Linux ppc64le Build #11850 [push] Zmodel using the device ... started at 12:40 |
Jenkins Linux amd64 Build #12833 [push] Zmodel using the device ... started at 11:30 |
Jenkins Linux amd64 Build #12833 [push] Zmodel using the device ... failed after 1 hr 17 min |
Jenkins Linux s390x Build #12857 [push] Zmodel using the device ... passed after 1 hr 22 min |
Jenkins Linux ppc64le Build #11850 [push] Zmodel using the device ... passed after 1 hr 42 min |
Use the new infrastructure to move to NNPA only operations that are deemed profitable.
Currently optional, enable with
--enable-zhigh-perf-model
flag