Print constants in a comment in CLIF output#4725
Conversation
cfallin
left a comment
cfallin
left a comment
There was a problem hiding this comment.
This is a reasonable change, thanks!
| let mut sep = " ; "; | ||
| for &arg in dfg.inst_args(inst) { | ||
| if let ValueDef::Result(src, _) = dfg.value_def(arg) { | ||
| if let UnaryImm { imm, .. } = dfg[src] { |
There was a problem hiding this comment.
Thanks for following up on this! What do you think about expanding this to UnaryIeee{32, 64} and UnaryBool? I think that would cover the f{32,64}const and bconst cases.
There was a problem hiding this comment.
Okay, done! That still leaves vconst but that's more complicated since those constants aren't inline in the instruction. I don't feel like figuring out how that works right now since it was enough of a pain fixing the tests for these changes already. I'd be happy to review a PR on top of this one if somebody feels like taking that on though.
There was a problem hiding this comment.
I changed my mind: it's good enough to just print which element of the constant pool is in a vector-typed value, and that was easy to do. I think that might actually be more useful than printing giant vector literals. So now there's output for vconst as well.
jameysharp
force-pushed
the
print-consts
branch
from
cc26f73 to
1191d06
Compare
When trying to read generated CLIF, it's nice to be able to see at a glance that some of the operands are defined by `iconst` and similar instructions, without having to go find each operand's definition manually.
jameysharp
force-pushed
the
print-consts
branch
from
1191d06 to
94553ab
Compare
github-actions
Bot
added
the
cranelift
3 participants
When trying to read generated CLIF, it's nice to be able to see at a glance that some of the operands are defined by
iconstinstructions, without having to go find each operand's definition manually.@afonso360 suggested this in #4721 (and @bjorn3 seconded the idea) but I think it's a good idea independent of that issue.
I'm not sure if this is the right implementation, though: I don't understand the intent of the
FuncWritertrait well enough to know if adding this to the sharedwrite_operandsfunction is the best place. Perhaps only some implementations ofFuncWritershould do this?Because of that uncertainty, I haven't fixed up any of the tests which fail due to not expecting these extra comments. So I expect that CI should currently be failing.