From f39b29f07f790cdb6d9d30e3e139bfe6f98160e1 Mon Sep 17 00:00:00 2001 From: vbtcl Date: Thu, 15 Oct 2026 19:10:22 -0700 Subject: [PATCH] Read local variables in place when evaluating selects Element, range and member selects evaluated their value operand by copying it, so every read of one element of a large local array or packed vector in a constant function copied the whole variable. Hsiao ECC matrix generation, for example, reads single bits of a 20,880-bit packed local and single elements of a 22,800-element unpacked local in its inner loops. When the value operand names a local variable of the current frame and the selectors can't write to any variable, the select now reads the variable in place. Selectors with side effects keep the copy, so the value is still read before the selector's side effects. Co-Authored-By: Claude Opus 5.5 --- .../slang/ast/expressions/MiscExpressions.h | 6 ++ source/ast/expressions/MiscExpressions.cpp | 23 ++++++ source/ast/expressions/SelectExpressions.cpp | 79 +++++++++++++++++-- tests/unittests/ast/EvalTests.cpp | 58 ++++++++++++++ 4 files changed, 160 insertions(+), 6 deletions(-) diff --git a/include/slang/ast/expressions/MiscExpressions.h b/include/slang/ast/expressions/MiscExpressions.h index 850ddafe3f..2326114444 100644 --- a/include/slang/ast/expressions/MiscExpressions.h +++ b/include/slang/ast/expressions/MiscExpressions.h @@ -60,6 +60,12 @@ class SLANG_EXPORT NamedValueExpression final : public ValueExpressionBase { LValue evalLValueImpl(EvalContext& context) const; bool isEquivalentImpl(const NamedValueExpression& rhs) const; + /// If this expression names a local variable of the current call frame, + /// evaluates it without copying the value: returns a pointer to the + /// variable's current value, or to an invalid value if the evaluation + /// failed. Returns nullptr, having evaluated nothing, for any other symbol. + const ConstantValue* evalLocalRef(EvalContext& context) const; + static bool isKind(ExpressionKind kind) { return kind == ExpressionKind::NamedValue; } private: diff --git a/source/ast/expressions/MiscExpressions.cpp b/source/ast/expressions/MiscExpressions.cpp index 6a39bae363..746ecb3c9d 100644 --- a/source/ast/expressions/MiscExpressions.cpp +++ b/source/ast/expressions/MiscExpressions.cpp @@ -385,6 +385,29 @@ ConstantValue NamedValueExpression::evalImpl(EvalContext& context) const { return nullptr; } +const ConstantValue* NamedValueExpression::evalLocalRef(EvalContext& context) const { + if (getConstant() || bad()) + return nullptr; + + switch (symbol.kind) { + case SymbolKind::Parameter: + case SymbolKind::EnumValue: + case SymbolKind::Specparam: + return nullptr; + default: + break; + } + + ConstantValue* v = context.findLocal(&symbol); + if (!v) + return nullptr; + + if (!checkConstant(context)) + return &ConstantValue::Invalid; + + return v; +} + LValue NamedValueExpression::evalLValueImpl(EvalContext& context) const { if (!checkConstant(context)) return nullptr; diff --git a/source/ast/expressions/SelectExpressions.cpp b/source/ast/expressions/SelectExpressions.cpp index 7c35a700da..6a3be3da99 100644 --- a/source/ast/expressions/SelectExpressions.cpp +++ b/source/ast/expressions/SelectExpressions.cpp @@ -13,8 +13,10 @@ #include "slang/ast/Lookup.h" #include "slang/ast/TypeProvider.h" #include "slang/ast/expressions/CallExpression.h" +#include "slang/ast/expressions/ConversionExpression.h" #include "slang/ast/expressions/LiteralExpressions.h" #include "slang/ast/expressions/MiscExpressions.h" +#include "slang/ast/expressions/OperatorExpressions.h" #include "slang/ast/symbols/ClassSymbols.h" #include "slang/ast/symbols/CoverSymbols.h" #include "slang/ast/symbols/SubroutineSymbols.h" @@ -211,8 +213,71 @@ bool ElementSelectExpression::isConstantSelect(EvalContext& context) const { return value().type->hasFixedRange() && !selector().eval(context).bad(); } +// True if evaluating the expression can't write to any variable. +static bool isSideEffectFree(const Expression& expr) { + switch (expr.kind) { + case ExpressionKind::IntegerLiteral: + case ExpressionKind::RealLiteral: + case ExpressionKind::TimeLiteral: + case ExpressionKind::UnbasedUnsizedIntegerLiteral: + case ExpressionKind::NullLiteral: + case ExpressionKind::UnboundedLiteral: + case ExpressionKind::StringLiteral: + case ExpressionKind::NamedValue: + return true; + case ExpressionKind::UnaryOp: { + auto& unary = expr.as(); + switch (unary.op) { + case UnaryOperator::Preincrement: + case UnaryOperator::Predecrement: + case UnaryOperator::Postincrement: + case UnaryOperator::Postdecrement: + return false; + default: + return isSideEffectFree(unary.operand()); + } + } + case ExpressionKind::BinaryOp: { + auto& binary = expr.as(); + return isSideEffectFree(binary.left()) && isSideEffectFree(binary.right()); + } + case ExpressionKind::Conversion: + return isSideEffectFree(expr.as().operand()); + case ExpressionKind::ElementSelect: { + auto& select = expr.as(); + return isSideEffectFree(select.value()) && isSideEffectFree(select.selector()); + } + case ExpressionKind::RangeSelect: { + auto& select = expr.as(); + return isSideEffectFree(select.value()) && isSideEffectFree(select.left()) && + isSideEffectFree(select.right()); + } + case ExpressionKind::MemberAccess: + return isSideEffectFree(expr.as().value()); + default: + return false; + } +} + +// Evaluates the value operand of a select. A local variable is read in place +// rather than copied whole, unless one of the selectors evaluated after it +// could write to it. +static const ConstantValue* evalSelectValue(const Expression& value, + std::initializer_list selectors, + EvalContext& context, ConstantValue& storage) { + if (value.kind == ExpressionKind::NamedValue && + std::ranges::all_of(selectors, [](auto sel) { return isSideEffectFree(*sel); })) { + if (auto ref = value.as().evalLocalRef(context)) + return ref; + } + + storage = value.eval(context); + return &storage; +} + ConstantValue ElementSelectExpression::evalImpl(EvalContext& context) const { - ConstantValue cv = value().eval(context); + ConstantValue storage; + auto& cv = *evalSelectValue(value(), {&selector()}, context, storage); if (!cv) return nullptr; @@ -257,7 +322,7 @@ ConstantValue ElementSelectExpression::evalImpl(EvalContext& context) const { if (valType.isString()) return cv.getSlice(range->left, range->right, nullptr); - return std::move(cv).at(size_t(range->left)); + return cv.at(size_t(range->left)); } LValue ElementSelectExpression::evalLValueImpl(EvalContext& context) const { @@ -670,7 +735,8 @@ bool RangeSelectExpression::isConstantSelect(EvalContext& context) const { } ConstantValue RangeSelectExpression::evalImpl(EvalContext& context) const { - ConstantValue cv = value().eval(context); + ConstantValue storage; + auto& cv = *evalSelectValue(value(), {&left(), &right()}, context, storage); if (!cv) return nullptr; @@ -1206,7 +1272,8 @@ static bool checkPackedUnionTag(const Type& valueType, const SVInt& val, uint32_ } ConstantValue MemberAccessExpression::evalImpl(EvalContext& context) const { - ConstantValue cv = value().eval(context); + ConstantValue storage; + auto& cv = *evalSelectValue(value(), {}, context, storage); if (!cv) return nullptr; @@ -1256,10 +1323,10 @@ ConstantValue MemberAccessExpression::evalImpl(EvalContext& context) const { else { int32_t io = (int32_t)field.bitOffset; int32_t width = (int32_t)type->getBitWidth(); - cv = cv.integer().slice(width + io - 1, io); + ConstantValue result = cv.integer().slice(width + io - 1, io); // Make sure sign and four-statedness are correct. - return cv.convertToInt(type->getBitWidth(), type->isSigned(), type->isFourState()); + return result.convertToInt(type->getBitWidth(), type->isSigned(), type->isFourState()); } } diff --git a/tests/unittests/ast/EvalTests.cpp b/tests/unittests/ast/EvalTests.cpp index f9921bd7ed..d8bf59e505 100644 --- a/tests/unittests/ast/EvalTests.cpp +++ b/tests/unittests/ast/EvalTests.cpp @@ -2684,3 +2684,61 @@ endfunction NO_SESSION_ERRORS; } + +TEST_CASE("Eval selects of local variables") { + ScriptSession session; + session.eval(R"( +function automatic int sum_unpacked; + int a[8]; + int b[2]; + int s = 0; + for (int i = 0; i < 8; i++) a[i] = i * 3; + for (int i = 0; i < 8; i++) s += a[i] + a[7 - i]; + b = a[2:3]; + return s + b[1]; +endfunction +)"); + CHECK(session.eval("sum_unpacked()").integer() == 177); + + session.eval(R"( +function automatic logic [5:0] packed_bits; + logic [3:0][7:0] p = {8'h12, 8'h34, 8'h56, 8'h78}; + return {p[3][4], p[2][1], p[1][7:4]}; +endfunction +)"); + CHECK(session.eval("packed_bits()").integer() == 37); + + session.eval(R"( +typedef struct { int x; int y[2]; } s_t; +function automatic int struct_member; + s_t s; + s.x = 5; + s.y[1] = 7; + return s.x * s.y[1]; +endfunction +)"); + CHECK(session.eval("struct_member()").integer() == 35); + + // The value is read before the selector's side effect. + session.eval(R"( +function automatic int side_effect_select; + int a[4] = '{0, 20, 30, 40}; + return a[a[0]++]; +endfunction +)"); + CHECK(session.eval("side_effect_select()").integer() == 0); + + session.eval(R"( +function automatic int oob_select; + int a[4] = '{1, 2, 3, 4}; + int i = 9; + return a[i]; +endfunction +)"); + CHECK(session.eval("oob_select()").integer() == 0); + + auto diags = session.getDiagnostics(); + INFO(report(diags)); + REQUIRE(diags.size() == 1); + CHECK(diags[0].code == diag::IndexOOB); +}