Support for lowering references to imported vars.
#5513
Conversation
Emit a var declaration when lowering a constant reference to a `var`, and when emitting the definition, convert that declaration into a definition. Model reference name bindings as being constants so that we can look through them when importing and when lowering.
toolchain/sem_ir/inst_kind.h
Outdated
| @@ -80,6 +80,10 @@ enum class InstConstantKind : int8_t { | |||
| // the constants block. | |||
| // TODO: Decide if this is the model we want for these cases. | |||
| Unique, | |||
| // Same as unique, except that the instruction may or may not be constant. | |||
There was a problem hiding this comment.
| // Same as unique, except that the instruction may or may not be constant. | |
| // Similar to `Unique`, except that the instruction may or may not be constant. |
It's different so not the same. ;)
There was a problem hiding this comment.
For me, "Same as X except Y" is saying the only difference is Y, whereas "Similar to X except Y" leaves open the possibility that there are other differences too (it's mostly similar to X, but in case Y it's not even similar).
How about:
| // Same as unique, except that the instruction may or may not be constant. | |
| // The instruction may be a unique constant, as described for `Unique`. | |
| // Otherwise the instruction is not constant. |
There was a problem hiding this comment.
FWIW, mostly I wanted "unique' backticked, the "same" doesn't bother me that much.
There was a problem hiding this comment.
FWIW, maybe also Unique -> AlwaysUnique?
toolchain/check/import_ref.cpp
Outdated
| static_assert( | ||
| InstT::Kind.constant_kind() == SemIR::InstConstantKind::Unique || | ||
| InstT::Kind.constant_kind() == | ||
| SemIR::InstConstantKind::ConditionalUnique, | ||
| "Use ResolveAsDeduplicated"); |
There was a problem hiding this comment.
Could this be a requires instead?
There was a problem hiding this comment.
Yes, it could be, but we can't provide a custom error message so easily that way. Are you thinking that we give the two ResolveAs... functions the same name again? If we keep them separate, a diagnostic that mentions the other function name seems at least a little valuable.
There was a problem hiding this comment.
Maybe I'm just ambivalent about the ResolveAsDeduplicated, on two ends:
- I think people will figure it out from a simple compile error, a comment seems equivalent (this isn't a high-use API).
- I don't see why the name changed, when
requirescould have achieved equivalent enforcement. But, I didn't suggest/review it so I don't want to push too much.
I mainly noted it, though, since I've recently bumped into issues with static_assert and having to migrate things to requires. :)
There was a problem hiding this comment.
But note, I also am hesitant to just churn code when I don't see a benefit. It's not like I think overloading would really fix an issue. I just am losing my love for static_assert when requires would suffice. :)
There was a problem hiding this comment.
Switched to a concept.
Co-authored-by: Jon Ross-Perkins <[email protected]>
Co-authored-by: Jon Ross-Perkins <[email protected]>
Co-authored-by: Carbon Infra Bot <[email protected]>
Convert static_asserts to concept checks.
Co-authored-by: Jon Ross-Perkins <[email protected]>
Previously we walked the global variables defined by the current file and emitted an LLVM global variable definition for each of them. Now instead, when emitting a constant reference to a global variable, we emit an LLVM global variable declaration, and we then subsequently walk the global variables defined by the current file and convert each of them from a declaration to a definition.
In order to make import of names of global variables work, add support for import of
var, as well as support for importingtuple_accessandtuple_patternin the case where thevarhas a tuple pattern in its declaration. Also treatbind_names that are reference bindings tovars as having the same constant reference value as theirvarso that we can properly import and lower them.