Misc. improvements to raw/debug SemIR output#6557
Misc. improvements to raw/debug SemIR output#6557geoffromer merged 5 commits intocarbon-language:trunkfrom
Conversation
geoffromer
commented
Jan 6, 2026
geoffromer
commented
- Distinguish attached vs. unattached constants.
- Add some missing value stores to the top-level output.
- Add missing fields to various Print methods.
- Distinguish attached vs. unattached constants. - Add some missing value stores to the top-level output. - Add missing fields to various Print methods.
zygoloid
left a comment
zygoloid
left a comment
There was a problem hiding this comment.
Some thoughts but none are blocking.
| out << " (unattached)"; | ||
| } |
There was a problem hiding this comment.
I'd be inclined to leave out the (unattached) part since that's the "normal" case, and doesn't even make sense for non-symbolic constants, but I don't feel strongly -- if you think this is a useful reminder for users of the dump output, I don't mind it. If you want to keep it, how about using (attached) and (unattached) for symbolic constants, and (concrete) for concrete constants?
For the (attached) case, could we also include the attachment information like we do in formatted SemIR? (For the dump output, maybe it's better to list the generic ID and index instead of / in addition to the instruction in the eval block.)
There was a problem hiding this comment.
I'd be inclined to leave out the
(unattached)part since that's the "normal" case, and doesn't even make sense for non-symbolic constants, but I don't feel strongly -- if you think this is a useful reminder for users of the dump output, I don't mind it.
Yeah, I think it's a useful reminder, so I'd prefer to keep it.
If you want to keep it, how about using
(attached)and(unattached)for symbolic constants, and(concrete)for concrete constants?
Good idea. Done.
For the
(attached)case, could we also include the attachment information like we do in formatted SemIR? (For the dump output, maybe it's better to list the generic ID and index instead of / in addition to the instruction in the eval block.)
Would that already be covered by the code above that prints the SymbolicConstant? If not, can you be more specific about what you mean by "attachment information"? I'm afraid my grasp on the whole concept of attachment remains extremely tenuous.
There was a problem hiding this comment.
Oh, I'd not looked at what we're doing there. Yeah:
carbon-lang/toolchain/sem_ir/constant.h
Lines 87 to 103 in 444c18d
But this is also already printing out whether the symbolic constant is attached or not, although perhaps in a way that's not so obvious. Maybe we could put that information in the print for the SymbolicConstant instead? Eg:
auto Print(llvm::raw_ostream& out) const -> void {
- out << "{inst: " << inst_id << ", generic: " << generic_id
- << ", index: " << index << ", kind: ";
+ out << "{inst: " << inst_id << ", attached: " << generic_id.has_value();
+ if (generic_id.has_value()) {
+ out << ", generic: " << generic_id << ", index: " << index;
+ }
+ out << ", kind: ";That might cover a few more cases where we're dumping SymbolicConstants?
There was a problem hiding this comment.
Oh, yeah, that's a good idea. I've gone with a somewhat different format, though (see one_file.carbon for how it looks in practice). What do you think?
There was a problem hiding this comment.
In case you didn't see, I responded to the remaining open comment thread. (Not sure if github would have notified you for that.)
On reflection I'm fine with this as-is if you prefer this approach (and approving on that basis), but I think including an attached field in the dump of a SymbolicConstant is at least worth considering as an alternative.
geoffromer
left a comment
geoffromer
left a comment
There was a problem hiding this comment.
In case you didn't see, I responded to the remaining open comment thread. (Not sure if github would have notified you for that.)
Yeah, I got the notification, I just hadn't got back round to it yet.
| out << " (unattached)"; | ||
| } |
There was a problem hiding this comment.
Oh, yeah, that's a good idea. I've gone with a somewhat different format, though (see one_file.carbon for how it looks in practice). What do you think?
zygoloid
left a comment
zygoloid
left a comment
There was a problem hiding this comment.
I'm OK with this, but as noted I'd prefer something that's more obviously valid YAML.
| // CHECK:STDOUT: symbolic_constant60000114: {inst: inst6000014E, generic: generic60000000, index: generic_inst_in_def6, kind: checked} | ||
| // CHECK:STDOUT: symbolic_constant60000115: {inst: inst60000154, generic: generic<none>, index: generic_inst<none>, kind: checked} | ||
| // CHECK:STDOUT: symbolic_constant60000116: {inst: inst60000154, generic: generic60000000, index: generic_inst_in_def7, kind: checked} | ||
| // CHECK:STDOUT: symbolic_constant0: {inst: inst60000013, kind: self, <unattached>} |
There was a problem hiding this comment.
Our output here is supposed to be valid YAML. To my mild surprise, this is in fact valid despite the omitted :, and is equivalent to {inst: inst60000013, kind: self, <unattached>: null}. But perhaps something that's more obviously valid would be better? I'd be fine with attached: false or attached: null or unattached: true (or with omitting the attached: portion entirely) for unattached constants.
2 participants
