Add a framework for typed InstIds. #4454
Conversation
| // Subtyped InstId variants. | ||
| #define CARBON_SEM_IR_INST_KIND(Name) Name##InstId, | ||
| #include "toolchain/sem_ir/inst_kind.def" |
There was a problem hiding this comment.
I'm concerned by this part: right now we have a few places where we're uniformly processing InstId operands of instructions by looking at the IdKind, and after this change those places will no longer visit the storage_id field on Temporary (example, example). Maybe we could map all the typed InstId kinds to IdKind::For<InstId>?
There was a problem hiding this comment.
PS: I was concerned that collapsing to IdKind::For<InstId> would affect the ability of NodeStack (and possibly others) to propagate a TypedInstId. This is what led the discussion below, where there's maybe not actually enough value, closing the PR.
Right now it doesn't affect much. Maybe I'm a little biased due to typed node IDs and extract.cpp performance, but I think the biggest risk for compile performance is if it sees broad use in typed insts. Whereas right now that's not an issue, the number of kinds they deal with right now is small. If these start seeing broad use in signatures, I would expect that the things dealing with typed inst reflection will slow down (due to less reuse), and overall we could see a significant slowdown in compile times. However, it's possibly premature to make assumptions in that direction. |
|
Per discussion, closing this PR. We went through current uses of |
I'm switching
Temporary::storage_idto aTemporaryStorageInstIdas an example of this. Other than that, I'm not sure whether there are obvious low hanging fruit to change -- I figured starting to add the capability would be helpful towards use in future code.Note, if you don't think this is more broadly helpful, I can abort this, but I think this is something you'd brought up so just trying to provide it while feeling a little blocked elsewhere.