Fix constant-folding into bit sizes for various recursive cases#5602
Fix constant-folding into bit sizes for various recursive cases#5602ChrisDodd wants to merge 1 commit into
Conversation
ChrisDodd
force-pushed
the
cdodd-bitcfold
branch
from
bec4351 to
3c04b15
Compare
| if (auto cst = init->to<IR::Constant>()) { | ||
| if (auto dtype = d->type->to<IR::Type_Bits>()) { | ||
| auto dtype = resolveType(d->type); | ||
| while (auto td = dtype->to<IR::Type_Typedef>()) dtype = resolveType(td->type); |
There was a problem hiding this comment.
Do we want to treat Type_NewType here similar as typedef?
There was a problem hiding this comment.
A NewType is a distinct type that is not (implicitly) convertable back to the type is it created from. Since the size (operand) of a bit<N> must be a numeric type, it can't be a Type_NewType without an explicit cast. So constfolding would need to fold the cast, converting it into a constant literal, implying you would not see a Type_NewType here for legal code.
There was a problem hiding this comment.
So using a Type_NewType const value and casting it (eg bit<(bit<8>)X> where X is a const) doesn't work, as this code does not recognize constants of a NewType, and does not store them in its 'constants' map, so they don't get folded. This is due to the clause at line 181:
} else if (!d->type->is<IR::Type_InfInt>()) {
// Don't fold this yet, we can't evaluate the cast.
return d;
I'm not entirely sure why it thinks it can't evaluate the cast yet, but it definitely requires more analysis.
For now, this fix at least covers some cases with typedefs that were not working.
ChrisDodd
force-pushed
the
cdodd-bitcfold
branch
2 times, most recently
from
088bdc6 to
85b74d7
Compare
|
I have clicked on the button to update this PR's branch to merge with latest version of main branch, but the button is showing a spinning icon for several minutes afterwards, so it is not clear whether it will complete that step. I have disabled the Fedora linux CI Github action, so if any change is made to this PR to cause it to re-run tests, that always-failing one should not be run again. |
I rebased and pushed, so it now all seems to pass -- just needs approval |
ChrisDodd
force-pushed
the
cdodd-bitcfold
branch
3 times, most recently
from
cd77d3a to
0b6b33c
Compare
Signed-off-by: Chris Dodd <cdodd@nvidia.com>
3 participants
This is a fix for problems with various patterns of constant expressions in the 'size' of a
bittype that need to be folded and resolved very early in the front-end (prior to typechecking) in order to allow typechecking to proceed. It reveals some warts about the way typing and reference lookup for typedefs works, largely limitations of the refMap/typeMap way of doing things. Using the ResolutionContext instead of the refMap can avoid these problems, but this change does not fully remove the refMap from the ConstantFolding pass, though that is something we might want to do.This trips over a subtle issue in reference lookup in general while running a Modifier or Transform. When a name resolution results in a node that has been visited and modified by the current visitor, should it return the original node (as it was prior to starting the pass) or the modified node. Currently it always returns the original node, as a number of passes rely on that, but sometimes one wants the modified node. It would be nice if there was a better way of handling this.