-
Notifications
You must be signed in to change notification settings - Fork 367
Add a CSE pass before --EmitONNXIR #3265
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
Conversation
Signed-off-by: Chen Tong <[email protected]>
Signed-off-by: Chen Tong <[email protected]>
src/Compiler/CompilerPasses.cpp
Outdated
// with --EmitONNXIR. | ||
// This pass is inserted before the instrumentation pass, which may add | ||
// instrumentation ops with external sideeffect. | ||
pm.addPass(mlir::createCSEPass()); |
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 wonder if symbol-dce
is more appropriate?: https://mlir.llvm.org/docs/Passes/#-symbol-dce
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, the symbolDCE also removes the symbols, though there is no symbol declaration at this stage in onnx-mlir.
I replaced the CSE with symbolDCE and removed the debugging parameter, which is always true now.
Signed-off-by: Chen Tong <[email protected]>
Signed-off-by: Chen Tong <[email protected]>
Signed-off-by: Chen Tong <[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.
It seems you accidentally removed CSE? CSE is needed to merge parallel same patterns into one.
@@ -168,7 +168,7 @@ void addONNXToZHighPasses(mlir::PassManager &pm) { | |||
pm.addPass(onnx_mlir::zhigh::createZHighConstPropagationPass()); | |||
|
|||
// Remove common sub-expressions. | |||
pm.addPass(mlir::createCSEPass()); |
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.
Was it removed by accident?
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, my mistake.
if (enableCSE) | ||
// Eliminate common sub-expressions before lowering to Krnl. | ||
// TODO: enable this by default when we make sure it works flawlessly. | ||
pm.addPass(mlir::createCSEPass()); |
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.
Looks like this was removed by accident since enableCSE=true
means we call pm.addPass(mlir::createCSEPass());
.
Signed-off-by: Chen Tong <[email protected]>
I found that this PR is not really needed. The size of remaining IR is caused by the embedding weights, simplification can not help. |
The purpose of this PR is to get a clearer output of --EmitONNXIR with unused ops removed. This is useful when a model is manually altered.
To me, the existing invocation of CSE pass in conversion from onnx to krnl could be removed. But I did not touch it in this PR.