Build a list of dependent constants to recompute in each instance of a generic. #4110

zygoloid · 2024-07-04T00:12:32Z

For each generic, build a list of instructions describing the computations we need to do when resolving an instance of the generic: this is a list of the instance-specific constants and types that the generic uses. Another way of viewing this list is as a block of Carbon SemIR code that is evaluated in order to form an instance of the generic -- this is referenced in the code as the "eval block" for the generic.

For each instruction in the generic whose type or value is a symbolic constant, replace that type or constant value with a symbolic reference that says "to find the actual type or value, look at index N in the list of values for the generic instance".

For an instruction with a symbolic constant value, we can just add that instruction to our list. For an instruction with a symbolic constant type, however, we may not have a corresponding instruction computing the type within the generic and may need to build a new instruction, but will reuse one where possible. In the case where we build a new instruction, we use the existing substitution code to build the type within the eval block.

For now, this transformation is only done in the declaration region of the generic, not in the definition region. Also, we map back from the symbolic references to the underlying constant value in a few places where we will eventually need to do a lookup into a generic instance, in order to avoid regressing the tests.

When forming a `ConstantId` for a symbolic constant, add storage to track the generic in which the constant was formed and the index within that generic. These fields are not yet populated.

generic instance.

…ration of a generic.

types and values from generic instances.

jonmeow

The implementation looks good, but I spent a lot of time examining the substitute/rebuild flow changes due to the lambdas. The flow seems fine (although may be worth considering a class with virtual functions instead of lambda parameters?), but a pass on comments seems like it could be helpful.

jonmeow · 2024-07-08T15:29:04Z

toolchain/sem_ir/type.h

+  // Returns whether two type IDs represent the same type. This includes the
+  // case where they might be in different generics and thus might have
+  // different ConstantIds, but are still symbolically equal.
+  auto EqualAcrossDeclarations(TypeId a, TypeId b) const -> bool {


FWIW, I'd lean towards IsEqual... or AreEqual... here, similar to IsComplete (for verb phrasing)

AreEqual... seems to work quite nicely with the value store accessors: ...types().AreEqual....

Done, and also renamed the corresponding function on the constant_values store.

jonmeow · 2024-07-08T15:34:48Z

toolchain/check/subst.h

@@ -5,6 +5,7 @@
 #ifndef CARBON_TOOLCHAIN_CHECK_SUBST_H_
 #define CARBON_TOOLCHAIN_CHECK_SUBST_H_

+#include "common/map.h"


I may be missing something, but why is this added? Maybe leftover while refactoring? Perhaps ironically to me, things like llvm::function_ref maybe should have an include.

Suggested change

#include "common/map.h"

#include "llvm/ADT/STLFunctionalExtras.h"

Yeah, left over from refactoring. Just removing the include since I'm also going to look at switching from lambdas to virtual functions.

jonmeow · 2024-07-08T15:46:19Z

toolchain/check/subst.cpp

+      auto inst_id = SemIR::InstId(arg);
+      if (!inst_id.is_valid()) {


I'll just note, this looks kind of weird versus line 66 in the delta. I don't know if it's worth switching formats since this is consistent with line 122 below, or you could switch both. Or don't do anything, it's probably not going to look as odd later in code, versus when I'm just seeing the delta.

Noted. I think this does look less weird in the full context of the file.

jonmeow · 2024-07-08T15:56:18Z

toolchain/check/subst.cpp

 // Pops the operands of the specified instruction off the worklist and rebuilds
 // the instruction with the updated operands.


This sounds like it would unconditionally call rebuild_fn: should the documentation be adjusted?

jonmeow · 2024-07-08T16:13:49Z

toolchain/check/subst.h

+// returns false, the instruction will be decomposed and substitution will be
+// performed recursively into its operands.


Most of the comments seem to be describing implementation constraints. From reading the implementation, I think this is different: is this a constraint on the caller, or is it a constraint on the implementation? If it's a constraint on the caller, perhaps it should be on SubstInst instead?

Changed to talk more about what the return value means and less about what SubstInst will do with it.

jonmeow · 2024-07-08T16:15:48Z

toolchain/check/subst.h

+using SubstRebuildFn = llvm::function_ref<
+    auto(SemIR::InstId orig_inst_id, SemIR::Inst new_inst)->SemIR::InstId>;
+
+// Performs substitution into `inst_id` and its operands recursively.


Maybe the flow should have some more comments here? From the comments:

subst_fn: "A function that performs any needed substitution into an instruction"
rebuild_fn: "A function that rebuilds an instruction after substitution."
SubstInst: "Performs substitution into inst_id and its operands recursively."

Is subst_fn called on all the operands of inst_id unconditionally [unconditional on direct, conditional on indirect], or only recursively [conditional on both direct and indirect]?

On line 237, "// No need to rebuild this instruction.", why do instructions that return false on subst_fn but have no operands not get rebuilt? (or, why are they allowed to return false when they don't need substitution?)

I've also commented on Rebuild, because the call to rebuild_fn looks conditional in a way I don't see explained here. [but note since that's going to be in the cpp file, may still be worth a note here]

Here, maybe something like:

Performs recursive substitution on instructions, starting with inst_id. Each instruction is passed to subst_fn:

On true, the instruction is a leaf.

On false, this recurses into the instruction's operands. After all operands are processed, then when [Rebuild condition], the instruction is passed to rebuild_fn.

But it may be worth looking at further than just that.

Did a pass over the comments.

jonmeow · 2024-07-08T16:20:09Z

toolchain/check/generic.cpp

+  for (size_t i = 0;
+       i != context.generic_region_stack().PeekDependentInsts().size(); ++i) {


Suggested change

for (size_t i = 0;

i != context.generic_region_stack().PeekDependentInsts().size(); ++i) {

for (auto i : llvm::seq(context.generic_region_stack().PeekDependentInsts().size())) {

Hm, yeah, while the collection could get reallocated (eg, by import ref handling), the number of instructions at this level on the stack really shouldn't change. Made this change and also added a CHECK.

zygoloid

The flow seems fine (although may be worth considering a class with virtual functions instead of lambda parameters?), but a pass on comments seems like it could be helpful.

Switched to a class with virtual functions as suggested. I think that is an improvement. I didn't find a really satisfying name for the class, but I think SubstInstCallbacks is at least clear as to its purpose.

zygoloid · 2024-07-08T22:02:50Z

toolchain/check/generic.cpp

+  for (size_t i = 0;
+       i != context.generic_region_stack().PeekDependentInsts().size(); ++i) {


Hm, yeah, while the collection could get reallocated (eg, by import ref handling), the number of instructions at this level on the stack really shouldn't change. Made this change and also added a CHECK.

zygoloid · 2024-07-08T22:06:36Z

toolchain/check/subst.cpp

+      auto inst_id = SemIR::InstId(arg);
+      if (!inst_id.is_valid()) {


Noted. I think this does look less weird in the full context of the file.

zygoloid · 2024-07-08T22:09:12Z

toolchain/check/subst.h

+// returns false, the instruction will be decomposed and substitution will be
+// performed recursively into its operands.


Changed to talk more about what the return value means and less about what SubstInst will do with it.

zygoloid · 2024-07-08T22:10:10Z

toolchain/check/subst.h

+using SubstRebuildFn = llvm::function_ref<
+    auto(SemIR::InstId orig_inst_id, SemIR::Inst new_inst)->SemIR::InstId>;
+
+// Performs substitution into `inst_id` and its operands recursively.


Did a pass over the comments.

jonmeow

Thanks! The edits are helpful for me.

As noted in chat, in my head I'd been thinking a full class for SubstInst logic, not just the callbacks, but this still seems a little easier to grasp.

zygoloid added 7 commits July 1, 2024 22:42

Store additional information for symbolic constants.

952dc44

When forming a `ConstantId` for a symbolic constant, add storage to track the generic in which the constant was formed and the index within that generic. These fields are not yet populated.

Review comments

2be7384

Build a list of instructions whose values need to be recomputed in a

5a3e025

generic instance.

Build lists of instructions to evaluate in order to resolve the decla…

a030ba8

…ration of a generic.

Apply some workarounds to keep tests passing until we start pulling

d49c679

types and values from generic instances.

Untemplate

904a40c

Another workaround to keep tests passing.

7f493c4

github-actions bot requested a review from geoffromer July 4, 2024 00:12

github-actions bot added the toolchain label Jul 4, 2024

Merge branch 'trunk' into toolchain-generic-2

604ce18

zygoloid requested review from jonmeow and removed request for geoffromer July 4, 2024 01:01

jonmeow approved these changes Jul 8, 2024

View reviewed changes

zygoloid added 4 commits July 8, 2024 20:52

Address some review comments.

067159d

Switch from a pair of function_refs to virtual functions.

b3f9600

Address review comment.

7668251

Tweak comment.

eeb1343

zygoloid commented Jul 8, 2024

View reviewed changes

zygoloid requested a review from jonmeow July 8, 2024 22:11

jonmeow approved these changes Jul 8, 2024

View reviewed changes

zygoloid added this pull request to the merge queue Jul 8, 2024

Merged via the queue into carbon-language:trunk with commit 7322a1e Jul 8, 2024
7 checks passed

zygoloid deleted the toolchain-generic-2 branch July 8, 2024 22:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build a list of dependent constants to recompute in each instance of a generic. #4110

Build a list of dependent constants to recompute in each instance of a generic. #4110

zygoloid commented Jul 4, 2024

jonmeow left a comment

jonmeow Jul 8, 2024

zygoloid Jul 8, 2024

jonmeow Jul 8, 2024

zygoloid Jul 8, 2024

jonmeow Jul 8, 2024

zygoloid Jul 8, 2024

jonmeow Jul 8, 2024

zygoloid Jul 8, 2024

jonmeow Jul 8, 2024

zygoloid Jul 8, 2024

jonmeow Jul 8, 2024 •

edited

Loading

zygoloid Jul 8, 2024

jonmeow Jul 8, 2024

zygoloid Jul 8, 2024

zygoloid left a comment •

edited

Loading

zygoloid Jul 8, 2024

zygoloid Jul 8, 2024

zygoloid Jul 8, 2024

zygoloid Jul 8, 2024

jonmeow left a comment

	#include "common/map.h"
	#include "llvm/ADT/STLFunctionalExtras.h"

		auto inst_id = SemIR::InstId(arg);
		if (!inst_id.is_valid()) {

		// Pops the operands of the specified instruction off the worklist and rebuilds
		// the instruction with the updated operands.

		// returns false, the instruction will be decomposed and substitution will be
		// performed recursively into its operands.

		for (size_t i = 0;
		i != context.generic_region_stack().PeekDependentInsts().size(); ++i) {

	for (size_t i = 0;
	i != context.generic_region_stack().PeekDependentInsts().size(); ++i) {
	for (auto i : llvm::seq(context.generic_region_stack().PeekDependentInsts().size())) {

Build a list of dependent constants to recompute in each instance of a generic. #4110

Build a list of dependent constants to recompute in each instance of a generic. #4110

Conversation

zygoloid commented Jul 4, 2024

jonmeow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonmeow Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zygoloid left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonmeow left a comment

Choose a reason for hiding this comment

jonmeow Jul 8, 2024 •

edited

Loading

zygoloid left a comment •

edited

Loading