Dump CheckIRId more explicitly for InstId tags #6172

danakj · 2025-10-07T14:45:17Z

We swap the labels for ImportIRId and CheckIRId, so that CheckIRId the short name "ir" as this is now part of other ids (included as a tag), and is dumped as part of the id. Currently this is only for InstId but other Ids are coming.

Then we make the dumping process of the CheckIRId a bit more typesafe by having the IdTag return a CheckIRId, and we print it in the usual way so that any special casing in the implementation of CheckIRId::Print will be used.

TODOs are left behind to improve the typesafety for IdTag.

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

Based on #6168, first commit is rename-irs

jonmeow · 2025-10-07T16:32:48Z

toolchain/base/value_store.h

+  }
+
+  struct IrAndIndex {
+    int32_t check_ir_id;


Why doesn't this use CheckIRId? TBH I'm even a little confused why int32_t for index, it seems like this could either be InstId, or a template like:

template <typename IdT> static auto DecomposeWithBestEffort(IdT id) -> std::pair<SemIR::CheckIRId, IdT> {

It's because this is in base/ not in sem_ir/. I wanted to make it use CheckIRId also but layering. We would have to move ValueStore to sem_ir/.

I also see on the PR introducing this that @zygoloid was thinking about future ValueStore usage where the tag would not even be a CheckIRId.

So maybe I should actually not call this check_ir_id at all here either, I will generalize it.

I think we could consider adding a type InstId::TagType = CheckIRId and then we could template the IdTag class and make things more typesafe - with a fallback to int32_t (or maybe a sentinel type?) for id types that don't specify a tag type. Maybe we could have a specialization of IdTag that more obviously does nothing in those cases.

jonmeow · 2025-10-07T16:33:26Z

toolchain/base/value_store.h

+    int32_t index;
+  };
+
+  static auto DecomposeWithBestEffort(int32_t tagged_index) -> IrAndIndex {


Maybe it's worth adding comments?

I believe @dwblaikie is going to follow up with some comments here

jonmeow · 2025-10-07T16:33:59Z

toolchain/base/value_store.h

+    return (llvm::reverseBits(2) & tagged_index) != 0;
+  }
+
+  struct IrAndIndex {


Don't we usually write "IR" instead of "Ir"?

jonmeow · 2025-10-07T16:34:52Z

toolchain/base/value_store.h

+    // zero-based, to make it a bit less likely this doesn't collide with
+    // anything else (though with the second-highest-bit-tagging this might not
+    // be needed).
    id_tag_ = llvm::reverseBits((((id_index + 1) << 1) | 1) << 1);


If you're editing constructors, perhaps this should be done via initializer list instead of assignment?

jonmeow · 2025-10-07T16:35:56Z

toolchain/sem_ir/ids.cpp

      IdBase::Print(out);
    } else {
-      out << "ir" << ir_id << ".inst" << simple_index;
+      out << CheckIRId::Label << check_ir_id << "." << Label << inst_id;


Maybe construct a CheckIRId here and use it for formatting? That way too, if we add special formatting (e.g. for Cpp) it would automatically get it here.

Great idea, thanks

jonmeow · 2025-10-07T16:45:00Z

toolchain/base/value_store.h

+  int32_t id_tag_ = 0;
+  int32_t initial_reserved_ids_ = std::numeric_limits<int32_t>::max();


Why did you switch away from the prior setup? It seemed safer, since there are only two constructors and both must set different values.

I think setting things on the fields is generally safer and preferred. It's easy to miss a field in the other form. If you do, this way they all get initialized to some default value at least.

danakj

PTAL

danakj · 2025-10-07T17:00:45Z

toolchain/base/value_store.h

+  }
+
+  struct IrAndIndex {
+    int32_t check_ir_id;


It's because this is in base/ not in sem_ir/. I wanted to make it use CheckIRId also but layering. We would have to move ValueStore to sem_ir/.

I also see on the PR introducing this that @zygoloid was thinking about future ValueStore usage where the tag would not even be a CheckIRId.

So maybe I should actually not call this check_ir_id at all here either, I will generalize it.

I think we could consider adding a type InstId::TagType = CheckIRId and then we could template the IdTag class and make things more typesafe - with a fallback to int32_t (or maybe a sentinel type?) for id types that don't specify a tag type. Maybe we could have a specialization of IdTag that more obviously does nothing in those cases.

danakj · 2025-10-07T17:01:04Z

toolchain/base/value_store.h

+    int32_t index;
+  };
+
+  static auto DecomposeWithBestEffort(int32_t tagged_index) -> IrAndIndex {


I believe @dwblaikie is going to follow up with some comments here

danakj · 2025-10-07T17:01:58Z

toolchain/base/value_store.h

+  int32_t id_tag_ = 0;
+  int32_t initial_reserved_ids_ = std::numeric_limits<int32_t>::max();


I think setting things on the fields is generally safer and preferred. It's easy to miss a field in the other form. If you do, this way they all get initialized to some default value at least.

danakj · 2025-10-07T19:05:54Z

toolchain/base/value_store.h

+    return (llvm::reverseBits(2) & tagged_index) != 0;
+  }
+
+  struct IrAndIndex {


danakj · 2025-10-07T19:13:10Z

toolchain/base/value_store.h

+    // zero-based, to make it a bit less likely this doesn't collide with
+    // anything else (though with the second-highest-bit-tagging this might not
+    // be needed).
    id_tag_ = llvm::reverseBits((((id_index + 1) << 1) | 1) << 1);


danakj · 2025-10-07T19:13:26Z

toolchain/sem_ir/ids.cpp

      IdBase::Print(out);
    } else {
-      out << "ir" << ir_id << ".inst" << simple_index;
+      out << CheckIRId::Label << check_ir_id << "." << Label << inst_id;


Great idea, thanks

danakj added 2 commits October 6, 2025 15:33

debug-irs

567d7a9

review

5b5b7b6

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

danakj requested a review from a team as a code owner October 7, 2025 14:45

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

danakj mentioned this pull request Oct 7, 2025

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

Open

jonmeow reviewed Oct 7, 2025

View reviewed changes

danakj added 4 commits October 7, 2025 14:51

wording

d3d29f9

rename-irs

df40a92

review

b3726b9

slightly-more-typesafe

82f33c8

danakj force-pushed the rename-irs branch from e098d8b to 82f33c8 Compare October 7, 2025 19:27

danakj commented Oct 7, 2025

View reviewed changes

danakj requested a review from jonmeow October 7, 2025 19:27

danakj changed the title ~~Swap the labels for ImportIRId and CheckIRId~~ Dump CheckIRId more explicitly for InstId tags Oct 7, 2025

		int32_t id_tag_ = 0;
		int32_t initial_reserved_ids_ = std::numeric_limits<int32_t>::max();

Dump CheckIRId more explicitly for InstId tags #6172

Are you sure you want to change the base?

Dump CheckIRId more explicitly for InstId tags #6172

Conversation

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

Uh oh!

Uh oh!

danakj commented Oct 7, 2025

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!

danakj left a comment

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 •

edited

Loading