Compute specific constant values. #4128
Conversation
When forming a generic instance (soon to be called a specific), evaluate the eval block of the generic to determine the values of any constants used in that specific. Include the computed results in the formatted SemIR output.
|
|
||
| // Information about the context within which we are performing evaluation. | ||
| struct EvalContext { | ||
| auto GetInContext(SemIR::ConstantId const_id) -> SemIR::ConstantId { |
There was a problem hiding this comment.
Comment? It's not clear to me how this differs from the others, and it looks like a public API (though, if this is class-ified, should it be private?)
| }; | ||
|
|
||
| // Information about the context within which we are performing evaluation. | ||
| struct EvalContext { |
There was a problem hiding this comment.
Noting how much of this is functions, should it be a class? Examining member uses, it looks like context and specific_id are part of the API, but file and specific_eval_info are not.
Though, thinking about this from a different perspective, had you considered making the static functions in eval.cpp be members of this class? I think we use the Context object elsewhere because we have a lot of different files we want to compose access from, but here, we have a scoped use-case. It'd also avoid the need to pass EvalContext as a parameter.
Without a class, file and sem_ir() in particular look like an odd match to me: why an accessor to return a public member?
There was a problem hiding this comment.
My intent is to make this into a class, but I'm trying to keep this PR minimal. Will send a follow-up with that change.
toolchain/check/eval.cpp
Outdated
| // A cached reference to the file, to avoid a double indirection when | ||
| // accessing its value stores. | ||
| SemIR::File& file = context.sem_ir(); |
There was a problem hiding this comment.
To note, this feels odd to me: it's unusual, and seems like it'd have a negligible effect on performance. Maybe I'm thinking about that wrong though?
The flipside is that all the internal sem_ir() calls look odd; if you keep this, maybe change them to reference the cached reference directly?
There was a problem hiding this comment.
Removed for now; we can re-add if we have evidence of this being an issue.
toolchain/check/eval.cpp
Outdated
| SpecificEvalInfo eval_info = { | ||
| .region = region, | ||
| .values = result, | ||
| }; | ||
| EvalContext eval_context = { | ||
| .context = context, | ||
| .specific_id = specific_id, | ||
| .specific_eval_info = &eval_info, | ||
| }; |
There was a problem hiding this comment.
FWIW, the extra struct from SpecificEvalInfo had made me expect it'd be replaced and/or separately used, but this is the only assignment I see other than nullptr. Given the way EvalContext is constructed, had you considered moving its members over, or perhaps making it std::optional instead of *?
There was a problem hiding this comment.
Originally I was concerned about this being large (containing a SmallVector) and I didn't want to include it in EvalContext for that reason, but that ended up not being a problem. Turned into a std::optional.
toolchain/check/eval.cpp
Outdated
| template <typename IdT> | ||
| auto GetConstantValueAsInst(IdT id) -> SemIR::Inst { | ||
| return insts().Get( | ||
| context.constant_values().GetInstId(GetConstantValue(id))); | ||
| } |
There was a problem hiding this comment.
This call confuses me a little: should it be taking a TypeId as parameter? It looks like the templated version isn't used.
(or will more callers be added? for one caller, maybe this function should be inlined)
There was a problem hiding this comment.
I've ungeneralized it. I'd prefer to not inline it; I found that having a name for this operation made it a lot easier to follow what the code was doing, and I am anticipating more callers.
Co-authored-by: Jon Ross-Perkins <[email protected]>
When forming a specific (previously called a generic instance), evaluate the eval block of the generic to determine the values of any constants used in that specific. The majority of the work here is updating eval.cpp so that it can use the results of prior evaluations in the same block when computing later values. Include the computed results in the formatted SemIR output. --------- Co-authored-by: Jon Ross-Perkins <[email protected]>
|
@Bonniemarie216 I see you approved a lot of different changes. Please don't do that anymore, because it's not contributing to the review, but it does create notifications for people who are monitoring repository activity. Thanks! |
When forming a specific (previously called a generic instance), evaluate the eval block of the generic to determine the values of any constants used in that specific. The majority of the work here is updating eval.cpp so that it can use the results of prior evaluations in the same block when computing later values.
Include the computed results in the formatted SemIR output.