Extend instrumentSignature to print data #3078
Conversation
Signed-off-by: Chen Tong <[email protected]>
Signed-off-by: Chen Tong <[email protected]>
Signed-off-by: Chen Tong <[email protected]>
src/Compiler/CompilerPasses.cpp
Outdated
| @@ -214,7 +214,7 @@ void addONNXToKrnlPasses(mlir::PassManager &pm, int optLevel, bool enableCSE, | |||
|
|
|||
| void addKrnlToAffinePasses(mlir::PassManager &pm) { | |||
| pm.addNestedPass<func::FuncOp>( | |||
| onnx_mlir::krnl::createConvertKrnlToAffinePass(enableParallel)); | |||
There was a problem hiding this comment.
You lost some changes from dev main. Please add them back.
src/Compiler/CompilerOptions.cpp
Outdated
| "Asterisk is also available.\n" | ||
| "e.g. \"onnx.*\" for all onnx operations.\n" | ||
| "If this option is started with \"onnx_node_name\"\n" | ||
| "the attribute of \"onnx_node_name\", instead of the op name\n" |
There was a problem hiding this comment.
missing the : in the pattern, as you are looking for this
std::string header = "onnx_node_name:";
Also you state that "the data values" will be printed. Is that the input data values, the output data values? Please specify.
| std::string header = "onnx_node_name:"; | ||
| std::cout << signaturePattern << "\n"; | ||
| bool useNodeName = false; | ||
| if (signaturePattern.rfind(header, 0) == 0) { |
There was a problem hiding this comment.
Can you explain the behavior here? If there is one match of "onnx_node_name", then all we use the node name for all ops?
There was a problem hiding this comment.
When the option starts with onnx_node_name:, we use attribute("onnx_node_name"), instead of op->getName(), to check all the op.
I am not sure what you mean by node name. I modified CompilerOption.cpp for this.
There was a problem hiding this comment.
I believe the code is not doing what you think it is doing.
if the pattern passed is "onnx.Add, onnx_node_name:one_specific_op", then the rfind will return true and thus useNodeName is set to true. Then for every op in the walk, we will fetch the data from the onnx_node_name for alll.
I think what you need to do is:
- use the old code to match with an op. if there is a match, then do the old print.
- else search the onnx_node_name attribute, if non null, prefix the attribute with "onnx_node_name:", search. If hit, then do the new print including the data.
There was a problem hiding this comment.
Then for every op in the walk, we will fetch the data from the onnx_node_name for all.
Yes, that's what I want. For all the op, either from onnx_node_name or op->getName(). We could separate the pattern for onnx_node_name and getName, and match each op accordingly. I did not do that: just keep the pattern description simple.
Signed-off-by: Chen Tong <[email protected]>
|
About the changes:
|
| auto dialect = op->getDialect(); | ||
| Location loc = op->getLoc(); |
There was a problem hiding this comment.
Did not check the logic in great detail, I assume that you used a small example mlir file and tried both the signature and the new functionality and that you were happy with the outputs.
Signed-off-by: Chen Tong <[email protected]>
|
Jenkins Linux amd64 Build #16330 [push] Extend instrumentSignatu... started at 19:28 |
|
Jenkins Linux s390x Build #16332 [push] Extend instrumentSignatu... started at 20:28 |
|
Jenkins Linux ppc64le Build #15313 [push] Extend instrumentSignatu... started at 20:38 |
|
Jenkins Linux s390x Build #16332 [push] Extend instrumentSignatu... failed after 1 hr 4 min |
|
Jenkins Linux amd64 Build #16330 [push] Extend instrumentSignatu... passed after 1 hr 24 min |
|
Jenkins Linux ppc64le Build #15313 [push] Extend instrumentSignatu... passed after 2 hr 33 min |
|
@jenkins-droid publish this please |
Extend instrumentSignature to be a tool that can be used to print the data of a particular onnx op at runtime.
Future work:
If we want to debug the crash inside the execution of an onnx op, there two changes are needed: