Correct the type for bytecode evaluation of enum value (via ColonRef) #1545
Conversation
| VLOG(5) << absl::StreamFormat( | ||
| "noting node: `%s` (%p) has constexpr value: `%s`", | ||
| const_expr->ToString(), const_expr, value.ToString()); | ||
| const_exprs_.insert_or_assign(const_expr, std::move(value)); |
There was a problem hiding this comment.
FYI, re review delay: The replacement of the const expr value sounds benign, however it breaks some other code outside this repository which I have yet to narrow down (time-slicing myself this week).
Highlevel: apparently in some for-loop in a proc, NoteConstExpr() is called with some array as value, and later for the same AstNode with a shorter array ... then things go south when some place attempts to access a non-existent index on the shorter array...
I've yet to determine the details. Will write a reproduction and fix when time permits.
There was a problem hiding this comment.
If you can get a trimmed down / publishable repro I'd be happy to debug and fix.
There was a problem hiding this comment.
Alright. The new https://github.com/google/xls/blob/main/xls/examples/memory_proc.x serves as a useful example for beginners, but it will also be a repo.
Essentially, the state, which is an array representing the 'memory' is replaced with a single value. Which might actually might be different problem somewhere else.
There was a problem hiding this comment.
Ok found the issue, it was for constexpr evaluation of arrays with ellipsis.
There were cases where we were confusing the number of members in the array literal with the array size even though there were ellipses.
Also removed accidental complexity in the course of making it consistent -- now ConstexprEvaluator calls into the bytecode intepreter directly instead of reimplementing another interpretation routine.
There was a problem hiding this comment.
Forgot to tag you @hzeller -- thanks for the reproducer!
Also helps to identify issue with #1545 #xls-build-gardener PiperOrigin-RevId: 666531259
cdleary
force-pushed
the
cdleary/2024-08-17-enum-in-struct
branch
from
bb66506
to
2fda13b
Compare
Fixes #1541
Note that this change found an issue only present in downstream code samples that was then surfaced as a new example. This subsequent issue was for constexpr evaluation of arrays with ellipsis.
There were cases where we were confusing:
Also removed accidental complexity in the course of making it consistent -- now
ConstexprEvaluatorcalls into the bytecode intepreter directly instead of reimplementing another interpretation routine forHandleArray.