Replace the desugared bit with an extra LocId range (carbon-language#5592)

This is to reduce the space consumption of the bit which only applies to
NodeId, and it also simplifies logic related to ImportIRInstId by
removing the need for index_without_flags.

Author: jonmeow

toolchain/sem_ir/ids.h

@@ -834,11 +834,9 @@ struct ImportIRInstId : public IdBase<ImportIRInstId> {
   static constexpr llvm::StringLiteral Label = "import_ir_inst";
   using ValueType = ImportIRInst;
 
-  // ImportIRInstId is restricted so that it can fit into LocId.
-  static constexpr int BitsWithNodeId = 30;
-
-  // The maximum ID, non-inclusive.
-  static constexpr int Max = (1 << BitsWithNodeId) - Parse::NodeId::Max - 2;
+  // The maximum ID, non-inclusive. This is constrained to fit inside LocId.
+  static constexpr int Max =
+      -(std::numeric_limits<int32_t>::min() + 2 * Parse::NodeId::Max + 1);
 
   constexpr explicit ImportIRInstId(int32_t index) : IdBase(index) {
     CARBON_DCHECK(index < Max, "Index out of range: {0}", index);
@@ -852,16 +850,15 @@ struct ImportIRInstId : public IdBase<ImportIRInstId> {
 //
 // The structure is:
 // - None: The standard NoneIndex for all Id types, -1.
-// - InstId: positive values including zero; a full 31 bits.
+// - InstId: Positive values including zero; a full 31 bits.
 //   - [0, 1 << 31)
-// - NodeId: negative values starting after None; the 24 bit NodeId range.
+// - NodeId: Negative values starting after None; the 24 bit NodeId range.
 //   - [-2, -2 - (1 << 24))
-// - ImportIRInstId: remaining negative values; after NodeId, fills out negative
-//   values to 30 bits.
-//   - [-2 - (1 << 24), -(1 << 30))
-//
-// In addition, one bit is used for flags: `DesugaredBit`.
-// Note that this can only be used with negative, non-`InstId` values.
+// - Desugared NodeId: Another 24 bit NodeId range.
+//   - [-2 - (1 << 24), -2 - (1 << 25))
+// - ImportIRInstId: Remaining negative values; after NodeId, fills out negative
+//   values.
+//   - [-2 - (1 << 25), -(1 << 31)]
 //
 // For desugaring, use `InstStore::GetLocIdForDesugaring()`.
 struct LocId : public IdBase<LocId> {
@@ -897,10 +894,9 @@ struct LocId : public IdBase<LocId> {
   auto AsDesugared() const -> LocId {
     // This should only be called for NodeId or ImportIRInstId (i.e. canonical
     // locations), but we only set the flag for NodeId.
-    CARBON_CHECK(kind() != Kind::InstId,
-                 "Use InstStore::GetLocIdForDesugaring");
-    if (kind() == Kind::NodeId) {
-      return LocId(index & ~DesugaredBit);
+    CARBON_CHECK(kind() != Kind::InstId, "Use InstStore::GetDesugaredLocId");
+    if (index <= FirstNodeId && index > FirstDesugaredNodeId) {
+      return LocId(index - Parse::NodeId::Max);
     }
     return *this;
   }
@@ -913,7 +909,7 @@ struct LocId : public IdBase<LocId> {
     if (index >= 0) {
       return Kind::InstId;
     }
-    if (index_without_flags() <= FirstImportIRInstId) {
+    if (index <= FirstImportIRInstId) {
       return Kind::ImportIRInstId;
     }
     return Kind::NodeId;
@@ -922,7 +918,7 @@ struct LocId : public IdBase<LocId> {
   // Returns true if the location corresponds to desugared instructions.
   // Requires a non-`InstId` location.
   auto is_desugared() const -> bool {
-    return (kind() == Kind::NodeId) && (index & DesugaredBit) == 0;
+    return index <= FirstDesugaredNodeId && index > FirstImportIRInstId;
   }
 
   // Returns the equivalent `ImportIRInstId` when `kind()` matches or is `None`.
@@ -934,7 +930,7 @@ struct LocId : public IdBase<LocId> {
       return ImportIRInstId::None;
     }
     CARBON_CHECK(kind() == Kind::ImportIRInstId, "{0}", index);
-    return ImportIRInstId(FirstImportIRInstId - index_without_flags());
+    return ImportIRInstId(FirstImportIRInstId - index);
   }
 
   // Returns the equivalent `InstId` when `kind()` matches or is `None`.
@@ -949,25 +945,22 @@ struct LocId : public IdBase<LocId> {
       return Parse::NodeId::None;
     }
     CARBON_CHECK(kind() == Kind::NodeId, "{0}", index);
-    return Parse::NodeId(FirstNodeId - index_without_flags());
+    if (index <= FirstDesugaredNodeId) {
+      return Parse::NodeId(FirstDesugaredNodeId - index);
+    } else {
+      return Parse::NodeId(FirstNodeId - index);
+    }
   }
 
   auto Print(llvm::raw_ostream& out) const -> void;
 
  private:
-  // Whether a location corresponds to desugared instructions. This only applies
-  // for `NodeId`.
-  static constexpr int32_t DesugaredBit = 1 << 30;
-
   // The value of the 0 index for each of `NodeId` and `ImportIRInstId`.
   static constexpr int32_t FirstNodeId = NoneIndex - 1;
-  static constexpr int32_t FirstImportIRInstId =
+  static constexpr int32_t FirstDesugaredNodeId =
       FirstNodeId - Parse::NodeId::Max;
-
-  auto index_without_flags() const -> int32_t {
-    CARBON_DCHECK(index < NoneIndex, "Only for NodeId and ImportIRInstId");
-    return index | DesugaredBit;
-  }
+  static constexpr int32_t FirstImportIRInstId =
+      FirstDesugaredNodeId - Parse::NodeId::Max;
 };
 
 // Polymorphic id for fields in `Any[...]` typed instruction category. Used for

toolchain/sem_ir/ids_test.cpp

@@ -24,13 +24,17 @@ TEST(IdsTest, LocIdValues) {
       static_cast<LocId>(InstId(std::numeric_limits<int32_t>::max())).index,
       Eq(std::numeric_limits<int32_t>::max()));
 
-  EXPECT_THAT(static_cast<LocId>(Parse::NodeId(0)).index, Eq(-2));
-  EXPECT_THAT(static_cast<LocId>(Parse::NodeId(Parse::NodeId::Max - 1)).index,
-              Eq(-2 - (1 << 24) + 1));
+  auto min_node_id = static_cast<LocId>(Parse::NodeId(0));
+  EXPECT_THAT(min_node_id.index, Eq(-2));
+  EXPECT_THAT(min_node_id.AsDesugared().index, Eq(-2 - (1 << 24)));
 
-  EXPECT_THAT(static_cast<LocId>(ImportIRInstId(0)).index, Eq(-2 - (1 << 24)));
+  auto max_node_id = static_cast<LocId>(Parse::NodeId(Parse::NodeId::Max - 1));
+  EXPECT_THAT(max_node_id.index, Eq(-2 - (1 << 24) + 1));
+  EXPECT_THAT(max_node_id.AsDesugared().index, Eq(-2 - (1 << 25) + 1));
+
+  EXPECT_THAT(static_cast<LocId>(ImportIRInstId(0)).index, Eq(-2 - (1 << 25)));
   EXPECT_THAT(static_cast<LocId>(ImportIRInstId(ImportIRInstId::Max - 1)).index,
-              Eq(-(1 << 30) + 1));
+              Eq(std::numeric_limits<int32_t>::min()));
 }
 
 // A standard parameterized test for (is_desugared, index).