-
Notifications
You must be signed in to change notification settings - Fork 66
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
Pass Attribute to debug function call #1422
base: main
Are you sure you want to change the base?
Conversation
aeca0d3
to
752e784
Compare
const std::map<std::string, std::string>& debugAttrMap) { | ||
#ifdef OP | ||
auto op = debugAttrMap.at("op"); | ||
auto sepBlockArgument = op.find("of type"); |
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.
What kind of of attribute generates text like this? It looks like it's a printed block argument (block argument at index #0 of type...
). Maybe there's an additional special case you can add to the map builder to avoid the string munging here and pass exactly what you want?
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.
What kind of of attribute generates text like this?
// Use AsmPrinter to print Value to print the defining op
auto ciphertext = op->getOperand(op->getNumOperands() - 1);
os << debugAttrMapName << R"(["op"] = ")" << ciphertext << "\"\n";
Here I called operator<<(raw_osstream&, Value)
to print the Value, which will be either <block argument> of type (!type) at index
or %0 = op %1...
, so it is MLIR AsmPrinter's behavior.
Maybe there's an additional special case you can add to the map builder to avoid the string munging here and pass exactly what you want?
Well I do can process this in Emitter by copying buffer from raw_ostream, but then that is too reliant on MLIR's behavior. Once the upstream changed the text string it has to adapt to that.
Another point I want to make is that, we should provide full op information to the debug function and let end user decide whether they want to see the verbose type (so the change to the debug function is just a template here), instead of stripping it earlier.
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.
Ah, so the complexity here is that you sometimes want to see the op name itself, and if it's not available then you want the type.
I feel like it may be simpler to just extract this info at map construction time...
os << debugAttrMapName << R"(["is_block_arg"] = ")" << isa<BlockArgument>(ciphertext) << "\"\n";
os << debugAttrMapName << R"(["value_type"] = ")" << ciphertext.getType() << "\"\n";
os << debugAttrMapName << R"(["op_name"] = ")" << [get op name or default value] << "\"\n";
os << debugAttrMapName << R"(["result_ssa_format"] = ")" << ciphertext << "\"\n";
You could still provide the raw string as a catch-all, but I feel that having string munging as the only mechanism to extract information you already know you want is rather brittle.
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.
Updated to have more information at construction time.
Though instead of seeing detailed asm like op with operand name and result name.
%ct = openfhe.mul_no_relin %arg0, %arg2, %arg3
...
%ct_0 = openfhe.relin %arg0, %ct
...
we only get the op name for convenience now.
Input
...
Input
...
openfhe.mul_no_relin
...
openfhe.relin
...
752e784
to
6a0a3c5
Compare
6a0a3c5
to
23c994f
Compare
Fixes #1415
Because heir-translate does not have the
func::FuncOp OpAsmOpInterface
hack some%arg0
is present instead of%evaluator
or%cc
. Should be fixed later when I getOpAsmTypeInterface
integrated upstream in MLIR (llvm/llvm-project#124700).For Openfhe, the debug output is quite verbose because of LWE type. For printing op in generic form by AsmPrinter we can not have type alias.resolved now with post-processing in debug functionExample
Lattigo
Openfhe