Skip to content

Commit

Permalink
Add new compat flag that allows self-determined streaming concatenati…
Browse files Browse the repository at this point in the history
…on expressions
  • Loading branch information
MikePopoloski committed Mar 8, 2024
1 parent 5159c17 commit a34e29b
Show file tree
Hide file tree
Showing 8 changed files with 85 additions and 40 deletions.
4 changes: 3 additions & 1 deletion bindings/python/CompBindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,9 @@ void registerCompilation(py::module_& m) {
.value("IgnoreUnknownModules", CompilationFlags::IgnoreUnknownModules)
.value("RelaxStringConversions", CompilationFlags::RelaxStringConversions)
.value("AllowRecursiveImplicitCall", CompilationFlags::AllowRecursiveImplicitCall)
.value("AllowBareValParamAssignment", CompilationFlags::AllowBareValParamAssignment);
.value("AllowBareValParamAssignment", CompilationFlags::AllowBareValParamAssignment)
.value("AllowSelfDeterminedStreamConcat",
CompilationFlags::AllowSelfDeterminedStreamConcat);

py::class_<CompilationOptions>(m, "CompilationOptions")
.def(py::init<>())
Expand Down
6 changes: 6 additions & 0 deletions docs/command-line-ref.dox
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,12 @@ Allow module instantiations to provide a single parameter value assignment witho
in parentheses. SystemVerilog doesn't permit this, but some tools allow it anyway so this flag
can be used for compatibility purposes.

`--allow-self-determined-stream-concat`

Allow self-determined streaming concatenation expressions. SystemVerilog only permits these
expressions in assignment-like contexts but some tools allow it anyway so this flag can be
used for compatibility purposes.

`--strict-driver-checking`

Perform strict driver checking, which currently means disabling procedural 'for' @ref loop-unroll
Expand Down
8 changes: 6 additions & 2 deletions include/slang/ast/Compilation.h
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,13 @@ enum class SLANG_EXPORT CompilationFlags {
AllowRecursiveImplicitCall = 1 << 10,

/// Allow module parameter assignments to elide the parentheses.
AllowBareValParamAssignment = 1 << 11
AllowBareValParamAssignment = 1 << 11,

/// Allow self-determined streaming concatenation expressions; normally these
/// can only be used in specific assignment-like contexts.
AllowSelfDeterminedStreamConcat = 1 << 12
};
SLANG_BITMASK(CompilationFlags, AllowBareValParamAssignment)
SLANG_BITMASK(CompilationFlags, AllowSelfDeterminedStreamConcat)

/// Contains various options that can control compilation behavior.
struct SLANG_EXPORT CompilationOptions {
Expand Down
2 changes: 1 addition & 1 deletion include/slang/ast/expressions/OperatorExpressions.h
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ class SLANG_EXPORT StreamingConcatenationExpression : public Expression {

static Expression& fromSyntax(Compilation& compilation,
const syntax::StreamingConcatenationExpressionSyntax& syntax,
const ASTContext& context, const Type* assignmentTarget);
const ASTContext& context);

static bool isKind(ExpressionKind kind) { return kind == ExpressionKind::Streaming; }

Expand Down
3 changes: 1 addition & 2 deletions source/ast/Expression.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -849,8 +849,7 @@ Expression& Expression::create(Compilation& compilation, const ExpressionSyntax&
break;
case SyntaxKind::StreamingConcatenationExpression:
result = &StreamingConcatenationExpression::fromSyntax(
compilation, syntax.as<StreamingConcatenationExpressionSyntax>(), context,
assignmentTarget);
compilation, syntax.as<StreamingConcatenationExpressionSyntax>(), context);
break;
case SyntaxKind::ElementSelectExpression:
result = &bindSelectExpression(compilation, syntax.as<ElementSelectExpressionSyntax>(),
Expand Down
66 changes: 34 additions & 32 deletions source/ast/expressions/OperatorExpressions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1751,13 +1751,9 @@ void ReplicationExpression::serializeTo(ASTSerializer& serializer) const {

Expression& StreamingConcatenationExpression::fromSyntax(
Compilation& comp, const StreamingConcatenationExpressionSyntax& syntax,
const ASTContext& context, const Type* assignmentTarget) {
const ASTContext& context) {

// The sole purpose of assignmentTarget here is to know whether this is a
// "destination" stream (i.e. an unpack operation) or whether it is a source / pack.
// Streaming concatenation is self-determined and its size/type should not be affected
// by assignmentTarget.
const bool isDestination = !assignmentTarget;
const bool isDestination = context.flags.has(ASTFlags::LValue);
const bool isRightToLeft = syntax.operatorToken.kind == TokenKind::LeftShift;
uint64_t sliceSize = 0;

Expand All @@ -1767,7 +1763,8 @@ Expression& StreamingConcatenationExpression::fromSyntax(
std::span<const StreamExpression>(), syntax.sourceRange()));
};

if (!context.flags.has(ASTFlags::StreamingAllowed)) {
if (!context.flags.has(ASTFlags::StreamingAllowed) &&
(isDestination || !comp.hasFlag(CompilationFlags::AllowSelfDeterminedStreamConcat))) {
context.addDiag(diag::BadStreamContext, syntax.operatorToken.location());
return badResult();
}
Expand Down Expand Up @@ -1811,33 +1808,24 @@ Expression& StreamingConcatenationExpression::fromSyntax(
uint64_t bitstreamWidth = 0;
SmallVector<StreamExpression, 4> buffer;
for (const auto argSyntax : syntax.expressions) {
Expression* arg;
if (assignmentTarget &&
argSyntax->expression->kind == SyntaxKind::StreamingConcatenationExpression) {
arg = &create(comp, *argSyntax->expression, context, ASTFlags::StreamingAllowed,
assignmentTarget);
}
else {
arg = &selfDetermined(comp, *argSyntax->expression, context,
ASTFlags::StreamingAllowed);
}

if (arg->bad())
auto& arg = selfDetermined(comp, *argSyntax->expression, context,
ASTFlags::StreamingAllowed);
if (arg.bad())
return badResult();

const Type* argType = arg->type;
const Type* argType = arg.type;
const Expression* withExpr = nullptr;
std::optional<bitwidth_t> constantWithWidth;
if (argSyntax->withRange) {
// The expression before the with can be any one-dimensional unpacked array
// (including a queue).
if (!argType->isUnpackedArray() || argType->isAssociativeArray() ||
!argType->getArrayElementType()->isFixedSize()) {
context.addDiag(diag::BadStreamWithType, arg->sourceRange);
context.addDiag(diag::BadStreamWithType, arg.sourceRange);
return badResult();
}

withExpr = &bindSelector(comp, *arg, *argSyntax->withRange->range,
withExpr = &bindSelector(comp, arg, *argSyntax->withRange->range,
context.resetFlags(ASTFlags::StreamingWithRange));
if (withExpr->bad())
return badResult();
Expand All @@ -1861,17 +1849,17 @@ Expression& StreamingConcatenationExpression::fromSyntax(
}

if (!argType->isBitstreamType(isDestination)) {
context.addDiag(diag::BadStreamExprType, arg->sourceRange) << *arg->type;
context.addDiag(diag::BadStreamExprType, arg.sourceRange) << *arg.type;
return badResult();
}

if (!Bitstream::checkClassAccess(*argType, context, arg->sourceRange))
if (!Bitstream::checkClassAccess(*argType, context, arg.sourceRange))
return badResult();
}

uint64_t argWidth = 0;
if (arg->kind == ExpressionKind::Streaming) {
argWidth = arg->as<StreamingConcatenationExpression>().getBitstreamWidth();
if (arg.kind == ExpressionKind::Streaming) {
argWidth = arg.as<StreamingConcatenationExpression>().getBitstreamWidth();
}
else if (withExpr) {
if (constantWithWidth) {
Expand All @@ -1895,14 +1883,28 @@ Expression& StreamingConcatenationExpression::fromSyntax(
return badResult();
}

buffer.push_back({arg, withExpr, constantWithWidth});
buffer.push_back({&arg, withExpr, constantWithWidth});
}

// Streaming concatenation has no explicit type. Use void to prevent problems when
// its type is passed to context-determined expressions.
return *comp.emplace<StreamingConcatenationExpression>(comp.getVoidType(), sliceSize,
bitstreamWidth, buffer.ccopy(comp),
syntax.sourceRange());
// So normally the type of a streaming concatenation is never inspected,
// since it can only be used in assignments and there is explicit handling
// of these expressions there. We use a void type for the result to represent
// this (also because otherwise what type can we use for e.g. non fixed-size
// streams). However, in VCS compat mode the error about requiring an assignment
// context can be silenced, so we need to come up with a real result type here,
// which we do by converting to a packed bit vector of bitstream width.
auto& result = *comp.emplace<StreamingConcatenationExpression>(
comp.getVoidType(), sliceSize, bitstreamWidth, buffer.ccopy(comp), syntax.sourceRange());

if (!context.flags.has(ASTFlags::StreamingAllowed)) {
// Cap the width so we don't overflow. The conversion will error for us
// since the target width will be less than the source width.
auto width = std::min(bitstreamWidth, (uint64_t)SVInt::MAX_BITS);
auto& type = comp.getType(bitwidth_t(width), IntegralFlags::FourState);
return convertAssignment(context, type, result, result.sourceRange.start());
}

return result;
}

ConstantValue StreamingConcatenationExpression::evalImpl(EvalContext& context) const {
Expand Down
6 changes: 5 additions & 1 deletion source/driver/Driver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,9 @@ void Driver::addStandardArgs() {
"Allow implicit call expressions to be recursive function calls.");
addCompFlag(CompilationFlags::AllowBareValParamAssignment, "--allow-bare-value-param-assigment",
"Allow module parameter assignments to elide the parentheses.");
addCompFlag(CompilationFlags::AllowSelfDeterminedStreamConcat,
"--allow-self-determined-stream-concat",
"Allow self-determined streaming concatenation expressions");
addCompFlag(CompilationFlags::StrictDriverChecking, "--strict-driver-checking",
"Perform strict driver checking, which currently means disabling "
"procedural 'for' loop unrolling.");
Expand Down Expand Up @@ -415,7 +418,8 @@ bool Driver::processOptions() {
CompilationFlags::RelaxEnumConversions,
CompilationFlags::RelaxStringConversions,
CompilationFlags::AllowRecursiveImplicitCall,
CompilationFlags::AllowBareValParamAssignment};
CompilationFlags::AllowBareValParamAssignment,
CompilationFlags::AllowSelfDeterminedStreamConcat};

for (auto flag : vcsCompatFlags) {
auto& option = options.compilationFlags.at(flag);
Expand Down
30 changes: 29 additions & 1 deletion tests/unittests/ast/ExpressionTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1578,7 +1578,7 @@ module sub(input byte b);
};

for (const auto& test : illegal) {
if (testBitstream(test.sv, test.msg) != 1) {
if (testBitstream(test.sv, test.msg) == 0) {
FAIL_CHECK(test.sv);
}
}
Expand Down Expand Up @@ -3163,3 +3163,31 @@ endmodule
CHECK(diags[0].code == diag::SelectAfterRangeSelect);
CHECK(diags[1].code == diag::SelectAfterRangeSelect);
}

TEST_CASE("Streaming concat in non-stream context") {
auto tree = SyntaxTree::fromText(R"(
module m;
logic [16777214:0] a;
int i = {<<{a, a}} + 1;
let known(sig) = (!($isunknown({>>{sig}})));
property known_prop(sig, clk);
@(clk) (!($isunknown({>>{sig}})));
endproperty
wire clk;
assert property (known_prop(known(clk), clk));
endmodule
)");

CompilationOptions options;
options.flags |= CompilationFlags::AllowSelfDeterminedStreamConcat;

Compilation compilation(options);
compilation.addSyntaxTree(tree);

auto& diags = compilation.getAllDiagnostics();
REQUIRE(diags.size() == 1);
CHECK(diags[0].code == diag::BadStreamSize);
}

0 comments on commit a34e29b

Please sign in to comment.