Make dumping work with the `irM.instN` tagged InstId format #6168

danakj · 2025-10-06T19:12:49Z

As of #5997, InstId now contains a tag of the CheckIRId inside it, which works fine for debugging when dumping a C++ variable, but did not work anymore when trying to dump other InstIds that had been dumped. Teach lldb's dump command to pull apart the ir1.inst2 format and change the MakeInstId methods in dump.cpp to take the CheckIRId as an input. Since you could now possibly dump inst ids from another IR, provide an overload in Check:: that allows for this. And CHECK in SemIR:: if you try to dump something from the wrong IR.

Now this works again:

(lldb) dump context ir1.inst21
ir1.inst21: {kind: ClassType, arg0: class0, arg1: specific<none>, type: type(TypeType)}
  - type: type(TypeType): type; {kind: TypeType, type: type(TypeType)}
  - value: concrete_constant(ir1.inst21)
  - loc: LocId(<none>)

jonmeow · 2025-10-06T19:45:42Z

Just to note, on this PR, CheckIRId uses "check_ir" and ImportIRId uses "ir". It sounds like you're saying InstId/IdTag is using "ir" for CheckIRId; had you considered aligning the prefix as part of adding a dependency on which it is?

danakj · 2025-10-06T20:15:44Z

Just to note, on this PR, CheckIRId uses "check_ir" and ImportIRId uses "ir". It sounds like you're saying InstId/IdTag is using "ir" for CheckIRId; had you considered aligning the prefix as part of adding a dependency on which it is?

I have used check_ir_id in the variable names I am introducing. Are you suggesting check_ir1.inst2 as the dump format? That feels like it's getting long to me...

That said, I would be just fine with that if we omitted the ir from the id printing when it's from the current IR. I do want to do that, but it requires us to plumb File into Print, which I think we need to have a discussion about separate from just making dumping work again.

jonmeow · 2025-10-06T20:23:13Z

Yes, that's right. The incorrect prefix seemed to be part of why you were confused here, and it confused me too, so I'd thought maybe it'd be worth the characters.

danakj · 2025-10-06T21:08:44Z

Yes, that's right. The incorrect prefix seemed to be part of why you were confused here, and it confused me too, so I'd thought maybe it'd be worth the characters.

I see, yeah the biggest confusion was that I thought it must be referring to a foreign IR if it was appearing in the id like that. But the id number didn't match the foreign file. I did not at all consider it could be referring to the current file.

jonmeow

To be sure, I have very strong concerns about the multiple different uses of "ir". Look for example at the YAML format in testdata -- line 91 and line 124 use "ir1" with different meaning. This seems like it'll be a significant debugging hindrance; I'm maybe too familiar with what the prefixes mean, but also do you expect people joining to be able to understand this?

Also, taking a step back to one thing you said earlier about "check_ir" feeling "long". Is the main use of this copy-paste from other debug output, or are you expecting you'll usually type it in from scratch?

Speaking for myself, I see three possible answers here:

Change IdTag to match CheckIRId
- ImportIRId -> "ir", CheckIRId -> "check_ir", IdTag -> "check_ir"
Change ImportIRId and IdTag to always be explicit
- ImportIRId -> "import_ir", CheckIRId -> "check_ir", IdTag -> "check_ir"
Swap ImportIRId and CheckIRId
- ImportIRId -> "import_ir", CheckIRId -> "ir", IdTag -> "ir"

Personally, I'd favor (2) actually -- we should probably always clarify because confusion isn't worth the shorter name. (I could mainly see (3) if typing this out is a common use-case) But, partly noting this if it's worth further discussion (I assume this wasn't discussed in detail since the review was reassigned to me).

jonmeow · 2025-10-06T21:44:37Z

scripts/lldbinit.py

-    if m := re.fullmatch("([a-z_]+)(\\d+)", args[1]):
-        if m[1] in id_types:
+    # Look for <irN>.<type><id> as a single argument.
+    if m := re.fullmatch("ir(\\d+)\\.([a-z_]+)(\\d+)", args[1]):


Had you considered making this optional (e.g.. (ir(\\d+)\\.)?) to make this a single regex?

The arguments to the MakeInstId function are different depending on if there's an ir or not. I guess I could look for the group being present and branch, but this feels at least as straightforward to me.

toolchain/check/dump.cpp

scripts/lldbinit.py

jonmeow · 2025-10-06T22:28:11Z

scripts/lldbinit.py

          a Parse::Context, or a Lex::TokenizeBuffer.
  EXPR is a C++ expression such as a variable name. Use `--` to prevent it from
       being treated as a TYPE and ID.
+  IR is the CheckIRId(N) in the form `irN`.


Should this mention that it only supports inst? That seems new, and doesn't appear to be otherwise documented.

That will become wrong in the future, so I don't feel that we should. Maybe I can say something about what the dumped ids look like instead.

Perhaps still document that it's a subset, and where to look for the supported list? Or change the approach, and just drop the IR on the floor when it's not used?

Right now reading TYPE documentation says everything is allowed, so as long as it's a subset I think it's worth clarifying that the disagreement between TYPE documentation and behavior.

Yep, I have added some clarification and mentioned InstId without saying it's the only one that will be like this.

jonmeow · 2025-10-06T22:29:02Z

scripts/lldbinit.py

-            if len(args) != 3:
+    # Look for <type><id> as a single argument.
+    if m := re.fullmatch("([a-z_]+)(\\d+)", args[1]):
+        if m[1] in untagged_id_types:


Are you dropping support for dumping inst17 without ir. prefixed?

That form does not work anymore, unfortunately. At the moment, trying to dump an inst id just crashes. The InstId needs an ir index to add as a tag.

While we could add the current file's ir index to it, at the moment we're printing them as irN.instM so I don't see a lot of point in consuming them in a different way.

I want to change that but it's something beyond just getting dumping working again.

You seem to be expecting that it's always irN, but it's conditional. See https://github.com/carbon-language/carbon-lang/blob/trunk/toolchain/sem_ir/ids.cpp#L21 or the example https://github.com/carbon-language/carbon-lang/blob/trunk/toolchain/check/testdata/basics/raw_sem_ir/multifile.carbon#L60.

It's either inst<none> or irN.instM, but you can't dump a none, so for our purposes here it's always irN.instM.

Did you look at the link?

https://github.com/carbon-language/carbon-lang/blob/trunk/toolchain/check/testdata/basics/raw_sem_ir/multifile.carbon#L60

// CHECK:STDOUT: inst14: {kind: Namespace, arg0: name_scope0, arg1: inst<none>, type: type(inst(NamespaceType))}

Oh, I did.. I was looking at the rhs at arg1. I don't know how we get an inst with a none for the CheckIRId like that, hm...

I see, that inst14 is PackageInstId:

carbon-lang/toolchain/sem_ir/typed_insts.h

Line 1243 in 49213b1

static constexpr InstId PackageInstId = InstId(SingletonInstKinds.size());

It's.. like a singleton but isn't a singleton. It is the only inst like this.

It looks like this +1 is what prevents that InstId from getting tagged on Add as well:

carbon-lang/toolchain/sem_ir/file.cpp

Line 39 in 49213b1

insts_(this, SingletonInstKinds.size() + 1),

Ah.. there's more InstIds constructed without a tag here, inside formatting, which I am struggling to understand at the moment:

carbon-lang/toolchain/sem_ir/formatter.cpp

Lines 1239 to 1251 in 49213b1

InstId stripped_inst_id(

import_ir.sem_ir->insts().GetRawIndex(import_ir_inst.inst_id()));

switch (loc_id.kind()) {

case LocId::Kind::None: {

out_ << stripped_inst_id << " [no loc]";

break;

}

case LocId::Kind::ImportIRInstId: {

// TODO: Probably don't want to format each indirection, but maybe

// reuse GetCanonicalImportIRInst?

out_ << stripped_inst_id << " [indirect]";

break;

}

These special cases are removed in #6173

toolchain/sem_ir/dump.cpp

danakj · 2025-10-07T14:21:23Z

To be sure, I have very strong concerns about the multiple different uses of "ir". Look for example at the YAML format in testdata -- line 91 and line 124 use "ir1" with different meaning. This seems like it'll be a significant debugging hindrance; I'm maybe too familiar with what the prefixes mean, but also do you expect people joining to be able to understand this?

Also, taking a step back to one thing you said earlier about "check_ir" feeling "long". Is the main use of this copy-paste from other debug output, or are you expecting you'll usually type it in from scratch?

Speaking for myself, I see three possible answers here:

Change IdTag to match CheckIRId

ImportIRId -> "ir", CheckIRId -> "check_ir", IdTag -> "check_ir"

Change ImportIRId and IdTag to always be explicit

ImportIRId -> "import_ir", CheckIRId -> "check_ir", IdTag -> "check_ir"

Swap ImportIRId and CheckIRId

ImportIRId -> "import_ir", CheckIRId -> "ir", IdTag -> "ir"

Personally, I'd favor (2) actually -- we should probably always clarify because confusion isn't worth the shorter name. (I could mainly see (3) if typing this out is a common use-case) But, partly noting this if it's worth further discussion (I assume this wasn't discussed in detail since the review was reassigned to me).

I thought about 3 as well after I left work last night. I think it might be the best option, because yeah sometimes you do have to type things - I tended to type inst31 or whatever a lot before. Which is kind of why I want to get rid of the irN prefix for the current file too.

If possible I would like to separate all of these concerns from getting debugging/dumping functioning again though. Right now, we are printing inst ids that we can not dump, because constructing an inst id in the debugger does so without a tag, and the functions have no way to provide the ir id for the tag.

Will address the other feedback as well, and will reassign this review to you (thanks for looking at it)

danakj

PTAL

scripts/lldbinit.py

danakj · 2025-10-07T14:23:12Z

scripts/lldbinit.py

          a Parse::Context, or a Lex::TokenizeBuffer.
  EXPR is a C++ expression such as a variable name. Use `--` to prevent it from
       being treated as a TYPE and ID.
+  IR is the CheckIRId(N) in the form `irN`.


That will become wrong in the future, so I don't feel that we should. Maybe I can say something about what the dumped ids look like instead.

scripts/lldbinit.py

danakj · 2025-10-07T14:25:24Z

scripts/lldbinit.py

-    if m := re.fullmatch("([a-z_]+)(\\d+)", args[1]):
-        if m[1] in id_types:
+    # Look for <irN>.<type><id> as a single argument.
+    if m := re.fullmatch("ir(\\d+)\\.([a-z_]+)(\\d+)", args[1]):


The arguments to the MakeInstId function are different depending on if there's an ir or not. I guess I could look for the group being present and branch, but this feels at least as straightforward to me.

danakj · 2025-10-07T14:26:32Z

scripts/lldbinit.py

-            if len(args) != 3:
+    # Look for <type><id> as a single argument.
+    if m := re.fullmatch("([a-z_]+)(\\d+)", args[1]):
+        if m[1] in untagged_id_types:


That form does not work anymore, unfortunately. At the moment, trying to dump an inst id just crashes. The InstId needs an ir index to add as a tag.

While we could add the current file's ir index to it, at the moment we're printing them as irN.instM so I don't see a lot of point in consuming them in a different way.

I want to change that but it's something beyond just getting dumping working again.

toolchain/check/dump.cpp

danakj · 2025-10-07T14:46:04Z

To be sure, I have very strong concerns about the multiple different uses of "ir". Look for example at the YAML format in testdata -- line 91 and line 124 use "ir1" with different meaning. This seems like it'll be a significant debugging hindrance; I'm maybe too familiar with what the prefixes mean, but also do you expect people joining to be able to understand this?
Also, taking a step back to one thing you said earlier about "check_ir" feeling "long". Is the main use of this copy-paste from other debug output, or are you expecting you'll usually type it in from scratch?
Speaking for myself, I see three possible answers here:

Change IdTag to match CheckIRId

ImportIRId -> "ir", CheckIRId -> "check_ir", IdTag -> "check_ir"

Change ImportIRId and IdTag to always be explicit

ImportIRId -> "import_ir", CheckIRId -> "check_ir", IdTag -> "check_ir"

Swap ImportIRId and CheckIRId

ImportIRId -> "import_ir", CheckIRId -> "ir", IdTag -> "ir"

Personally, I'd favor (2) actually -- we should probably always clarify because confusion isn't worth the shorter name. (I could mainly see (3) if typing this out is a common use-case) But, partly noting this if it's worth further discussion (I assume this wasn't discussed in detail since the review was reassigned to me).

I thought about 3 as well after I left work last night. I think it might be the best option, because yeah sometimes you do have to type things - I tended to type inst31 or whatever a lot before. Which is kind of why I want to get rid of the irN prefix for the current file too.

If possible I would like to separate all of these concerns from getting debugging/dumping functioning again though. Right now, we are printing inst ids that we can not dump, because constructing an inst id in the debugger does so without a tag, and the functions have no way to provide the ir id for the tag.

Will address the other feedback as well, and will reassign this review to you (thanks for looking at it)

I have done the third one in #6172

danakj requested a review from a team as a code owner October 6, 2025 19:12

danakj requested review from josh11b and removed request for a team October 6, 2025 19:12

github-actions bot added infrastructure toolchain labels Oct 6, 2025

debug-irs

567d7a9

danakj force-pushed the debug-irs branch from 09f60b6 to 567d7a9 Compare October 6, 2025 19:33

josh11b requested review from jonmeow and removed request for josh11b October 6, 2025 20:24

jonmeow reviewed Oct 6, 2025

View reviewed changes

toolchain/sem_ir/dump.cpp Outdated Show resolved Hide resolved

review

5b5b7b6

danakj commented Oct 7, 2025

View reviewed changes

danakj requested a review from jonmeow October 7, 2025 14:31

danakj mentioned this pull request Oct 7, 2025

Swap the labels for ImportIRId and CheckIRId #6172

Open

	InstId stripped_inst_id(
	import_ir.sem_ir->insts().GetRawIndex(import_ir_inst.inst_id()));
	switch (loc_id.kind()) {
	case LocId::Kind::None: {
	out_ << stripped_inst_id << " [no loc]";
	break;
	}
	case LocId::Kind::ImportIRInstId: {
	// TODO: Probably don't want to format each indirection, but maybe
	// reuse GetCanonicalImportIRInst?
	out_ << stripped_inst_id << " [indirect]";
	break;
	}

Make dumping work with the irM.instN tagged InstId format #6168

Are you sure you want to change the base?

Make dumping work with the irM.instN tagged InstId format #6168

Conversation

danakj commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonmeow commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danakj commented Oct 6, 2025

Uh oh!

jonmeow commented Oct 6, 2025

Uh oh!

danakj commented Oct 6, 2025

Uh oh!

jonmeow left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danakj commented Oct 7, 2025

Uh oh!

danakj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danakj commented Oct 7, 2025

Uh oh!

Uh oh!

Make dumping work with the `irM.instN` tagged InstId format #6168

Make dumping work with the `irM.instN` tagged InstId format #6168

danakj commented Oct 6, 2025 •

edited

Loading

jonmeow commented Oct 6, 2025 •

edited

Loading

jonmeow left a comment •

edited

Loading