Avoid resolving the decl block for specifics in imported instructions #5517
Conversation
Co-authored-by: Jon Ross-Perkins <[email protected]>
|
I resolved the TODO in ResolveSpecificDeclForInst by only not resolving specific decls for self-specifics. A generic's self specific has its decl block explicitly resolved in FinishGenericDecl(), and that is the only specific that should be appearing inside dependent instructions that refers to the being-built generic. |
| ((values[SemIR::IdKind::template For<Types>.ToIndex()] = | ||
| HasGetConstantValueOverload<Types>), |
There was a problem hiding this comment.
The way this is initializing from a concept seems weird. I would've thought you could write something like:
constexpr std::array<bool, SemIR::IdKind::NumTypes> Values = {
((HasGetConstantValueOverload<Types>), ...)};
But that doesn't work. I don't know if you have ideas -- I also would suggest spending against much time on this, it seems minor. I assume you're using the struct to duck the nullptr thing I was doing, which seems fine though the need for a forward declaration seems like an odd quirk of C++.
There was a problem hiding this comment.
I think that doesn't work because Types does not include Invalid and None, so the RHS will be incorrect (wrong size, possibly wrong positions). And yeah, I was trying to avoid the nullptr dance :)
There was a problem hiding this comment.
That's why NumTypes. ;)
RHS should have right size and positions
There was a problem hiding this comment.
Ah yeah I see.
I think this doesn't work because (x,...) is very different than writing x,y,z. The operator comma is applied to each value in the fold expression, it doesn't expand like a macro would leaving behind all the elements from the fold.
Regardless, using NumTypes is a little improvement here, doing that.
There was a problem hiding this comment.
To just note, NumTypes may not work as-is. You'd need to still handle None, which is used for zero- and single- argument instructions (in ArgAndKind). Though you could also just handle that (and Invalid) directly in the switch.
(like I said, I could easily get the init to work)
|
(sorry, was playing with changes and accidentally did a push -- shouldn't have made any modifications) |
5aea18f
Tinkering with #5517, splitting out this suggestion to try to avoid delaying merge. I figured out what I was missing on the variadiac expansion. :) (and also realized the struct could probably be a function)
…carbon-language#5517) Move the operation of resolving the specific decl block from `GetConstantValue()` to `TryEvalTypedInst()`, with is now happening after replacing the fields of the instruction with new constant values, but before running the evaluation of the instruction. Since imported instructions are not evaluated, this avoids resolving the specific decl block from imported instructions, resolving a TODO in `AddImportedConstant()`. Now `AddImportedConstant()` can replace constant values in its fields without having to worry about that operation resolving any specific decl blocks. We get to add a new TODO however, to explain why we still need a special case in resolving specific decl blocks for handling `Impl` construction. The witness table contains instructions with specifics referring to the generic self of the impl declaration. But the table must be constructed before the impl's generic is finished, in order to make the instructions dependent for the generic. But then resolving the specific decl block can't be done when the instructions are created and evaluated, as that requires a finished generic. --------- Co-authored-by: Jon Ross-Perkins <[email protected]>
Tinkering with carbon-language#5517, splitting out this suggestion to try to avoid delaying merge. I figured out what I was missing on the variadiac expansion. :) (and also realized the struct could probably be a function)
Move the operation of resolving the specific decl block from
GetConstantValue()toTryEvalTypedInst(), with is now happening after replacing the fields of the instruction with new constant values, but before running the evaluation of the instruction. Since imported instructions are not evaluated, this avoids resolving the specific decl block from imported instructions, resolving a TODO inAddImportedConstant(). NowAddImportedConstant()can replace constant values in its fields without having to worry about that operation resolving any specific decl blocks.We get to add a new TODO however, to explain why we still need a special case in resolving specific decl blocks for handling
Implconstruction. The witness table contains instructions with specifics referring to the generic self of the impl declaration. But the table must be constructed before the impl's generic is finished, in order to make the instructions dependent for the generic. But then resolving the specific decl block can't be done when the instructions are created and evaluated, as that requires a finished generic.