From 59b416f9b4a79e83b295793fd4807d7ca96c51c2 Mon Sep 17 00:00:00 2001 From: Zachary Snow Date: Sun, 17 Jul 2022 20:32:56 -0400 Subject: [PATCH] isolate interface name resolution checks --- CHANGELOG.md | 1 + src/Convert/Interface.hs | 164 ++++++++++++++++-------- test/core/interface_check_same.sv | 107 ++++++++++++++++ test/core/interface_check_same.v | 97 ++++++++++++++ test/error/interface_bad_expr_arr.sv | 16 +++ test/error/interface_bad_expr_module.sv | 12 ++ 6 files changed, 344 insertions(+), 53 deletions(-) create mode 100644 test/core/interface_check_same.sv create mode 100644 test/core/interface_check_same.v create mode 100644 test/error/interface_bad_expr_arr.sv create mode 100644 test/error/interface_bad_expr_module.sv diff --git a/CHANGELOG.md b/CHANGELOG.md index 5eb83d5..fedd040 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ * Fixed signed `struct` fields being converted to unsigned expressions when accessed directly * Fixed conversion of casts using structs containing multi-dimensional fields +* Fixed incorrect name resolution conflicts raised during interface inlining ## v0.0.9 diff --git a/src/Convert/Interface.hs b/src/Convert/Interface.hs index d529711..3cf65ae 100644 --- a/src/Convert/Interface.hs +++ b/src/Convert/Interface.hs @@ -346,8 +346,10 @@ inlineInstance global ranges modportBindings items partName items' = evalScoper $ scopeModuleItems scoper partName $ map (traverseNestedModuleItems rewriteItem) $ if null modportBindings - then items ++ [typeModport, dimensionModport, bundleModport] - else items + then itemsChecked ++ infoModports + else itemsChecked + itemsChecked = checkBeforeInline global partName items checkErrMsg + infoModports = [typeModport, dimensionModport, bundleModport] scoper = scopeModuleItem traverseDeclM traverseModuleItemM traverseGenItemM traverseStmtM @@ -430,8 +432,8 @@ inlineInstance global ranges modportBindings items partName -- LHSs are replaced using simple substitutions traverseLHSM :: LHS -> Scoper () LHS traverseLHSM = - embedScopes tagLHS >=> - embedScopes replaceLHS + fmap replaceLHS . + embedScopes tagLHS tagLHS :: Scopes () -> LHS -> LHS tagLHS scopes lhs | lookupElem scopes lhs /= Nothing = @@ -446,25 +448,24 @@ inlineInstance global ranges modportBindings items partName then LHSDot scopedInstanceLHS y else LHSDot (LHSIdent x) y renamePartLHS lhs = traverseSinglyNestedLHSs renamePartLHS lhs - replaceLHS :: Scopes () -> LHS -> LHS - replaceLHS _ (LHSDot lhs "@") = lhs - replaceLHS local (LHSDot (LHSBit lhs elt) field) = + replaceLHS :: LHS -> LHS + replaceLHS (LHSDot lhs "@") = lhs + replaceLHS (LHSDot (LHSBit lhs elt) field) = case lookup (LHSDot (LHSBit lhs Tag) field) lhsReplacements of Just resolved -> replaceLHSArrTag elt resolved - Nothing -> LHSDot (replaceLHS local $ LHSBit lhs elt) field - replaceLHS local lhs = + Nothing -> LHSDot (replaceLHS $ LHSBit lhs elt) field + replaceLHS lhs = case lookup lhs lhsReplacements of Just lhs' -> lhs' - Nothing -> checkExprResolution local (lhsToExpr lhs) $ - traverseSinglyNestedLHSs (replaceLHS local) lhs + Nothing -> traverseSinglyNestedLHSs replaceLHS lhs replaceLHSArrTag :: Expr -> LHS -> LHS replaceLHSArrTag = traverseNestedLHSs . (traverseLHSExprs . replaceArrTag) -- top-level expressions may be modports bound to other modports traverseExprM :: Expr -> Scoper () Expr traverseExprM = - embedScopes tagExpr >=> - embedScopes replaceExpr + fmap replaceExpr . + embedScopes tagExpr tagExpr :: Scopes () -> Expr -> Expr tagExpr scopes expr | lookupElem scopes expr /= Nothing = @@ -479,38 +480,35 @@ inlineInstance global ranges modportBindings items partName then Dot scopedInstanceExpr y else Dot (Ident x) y renamePartExpr expr = visitExprsStep renamePartExpr expr - replaceExpr :: Scopes () -> Expr -> Expr - replaceExpr _ (Dot expr "@") = expr - replaceExpr local (Ident x) = + replaceExpr :: Expr -> Expr + replaceExpr (Dot expr "@") = expr + replaceExpr (Ident x) = case lookup x modportBindings of Just (_, m) -> m - Nothing -> checkExprResolution local (Ident x) (Ident x) - replaceExpr local expr = - replaceExpr' local expr - replaceExpr' :: Scopes () -> Expr -> Expr - replaceExpr' _ (Dot expr "@") = expr - replaceExpr' local (Dot (Bit expr elt) field) = + Nothing -> Ident x + replaceExpr expr = + replaceExpr' expr + replaceExpr' :: Expr -> Expr + replaceExpr' (Dot expr "@") = expr + replaceExpr' (Dot (Bit expr elt) field) = case lookup (Dot (Bit expr Tag) field) exprReplacements of - Just resolved -> replaceArrTag (replaceExpr' local elt) resolved - Nothing -> Dot (replaceExpr' local $ Bit expr elt) field - replaceExpr' local (Bit expr elt) = + Just resolved -> replaceArrTag (replaceExpr' elt) resolved + Nothing -> Dot (replaceExpr' $ Bit expr elt) field + replaceExpr' (Bit expr elt) = case lookup (Bit expr Tag) exprReplacements of - Just resolved -> replaceArrTag (replaceExpr' local elt) resolved - Nothing -> Bit (replaceExpr' local expr) (replaceExpr' local elt) - replaceExpr' local expr@(Dot Ident{} _) = + Just resolved -> replaceArrTag (replaceExpr' elt) resolved + Nothing -> Bit (replaceExpr' expr) (replaceExpr' elt) + replaceExpr' expr@(Dot Ident{} _) = case lookup expr exprReplacements of Just expr' -> expr' - Nothing -> checkExprResolution local expr $ - visitExprsStep (replaceExprAny local) expr - replaceExpr' local (Ident x) = - checkExprResolution local (Ident x) (Ident x) - replaceExpr' local expr = replaceExprAny local expr - replaceExprAny :: Scopes () -> Expr -> Expr - replaceExprAny local expr = + Nothing -> visitExprsStep replaceExprAny expr + replaceExpr' (Ident x) = Ident x + replaceExpr' expr = replaceExprAny expr + replaceExprAny :: Expr -> Expr + replaceExprAny expr = case lookup expr exprReplacements of Just expr' -> expr' - Nothing -> checkExprResolution local expr $ - visitExprsStep (replaceExpr' local) expr + Nothing -> visitExprsStep replaceExpr' expr replaceArrTag :: Expr -> Expr -> Expr replaceArrTag replacement Tag = replacement replaceArrTag replacement expr = @@ -529,22 +527,11 @@ inlineInstance global ranges modportBindings items partName . traverseExprTypes (traverseNestedTypes typeMapper) where typeMapper = traverseTypeExprs exprMapper - checkExprResolution :: Scopes () -> Expr -> a -> a - checkExprResolution local expr = - if not (exprResolves local expr) && exprResolves global expr - then - scopedError local $ "inlining instance \"" ++ instanceName - ++ "\" of " ++ inlineKind ++ " \"" ++ partName - ++ "\" would make expression \"" ++ show expr - ++ "\" used in \"" ++ instanceName - ++ "\" resolvable when it wasn't previously" - else id - - exprResolves :: Scopes a -> Expr -> Bool - exprResolves local (Ident x) = - isJust (lookupElem local x) || isLoopVar local x - exprResolves local expr = - isJust (lookupElem local expr) + checkErrMsg :: String -> String + checkErrMsg exprStr = "inlining instance \"" ++ instanceName + ++ "\" of " ++ inlineKind ++ " \"" ++ partName + ++ "\" would make expression \"" ++ exprStr ++ "\" used in \"" + ++ instanceName ++ "\" resolvable when it wasn't previously" -- unambiguous reference to the current instance scopedInstanceRaw = accessesToExpr $ localAccesses global instanceName @@ -723,3 +710,74 @@ sliceLo :: PartSelectMode -> Range -> Expr sliceLo NonIndexed (l, r) = endianCondExpr (l, r) r l sliceLo IndexedPlus (base, _) = base sliceLo IndexedMinus (base, len) = BinOp Add (BinOp Sub base len) (RawNum 1) + +-- check for cases where an expression in an inlined part only resolves after +-- inlining, potentially hiding a design error +checkBeforeInline :: Scopes a -> Identifier -> [ModuleItem] + -> (String -> String) -> [ModuleItem] +checkBeforeInline global partName items checkErrMsg = + evalScoper $ scopeModuleItems scoper partName $ items + where + scoper = scopeModuleItem + checkDecl checkModuleItem checkGenItem checkStmt + + checkDecl :: Decl -> Scoper () Decl + checkDecl decl = do + case decl of + Variable _ _ x _ _ -> insertElem x () + Net _ _ _ _ x _ _ -> insertElem x () + Param _ _ x _ -> insertElem x () + ParamType _ x _ -> insertElem x () + CommentDecl{} -> return () + traverseDeclExprsM checkExpr decl + + checkModuleItem :: ModuleItem -> Scoper () ModuleItem + checkModuleItem item@(Instance _ _ x _ _) = + insertElem x () >> traverseExprsM checkExpr item + checkModuleItem item = + traverseExprsM checkExpr item >>= + traverseLHSsM checkLHS + + checkGenItem :: GenItem -> Scoper () GenItem + checkGenItem = traverseGenItemExprsM checkExpr + + checkStmt :: Stmt -> Scoper () Stmt + checkStmt = + traverseStmtExprsM checkExpr >=> + traverseStmtLHSsM checkLHS + + checkExpr :: Expr -> Scoper () Expr + checkExpr = embedScopes checkExprResolutionId + + checkLHS :: LHS -> Scoper () LHS + checkLHS = embedScopes checkLHSResolutionId + + checkLHSResolutionId :: Scopes () -> LHS -> LHS + checkLHSResolutionId local lhs = checkExprResolution local expr lhs + where expr = lhsToExpr lhs + + checkExprResolutionId :: Scopes () -> Expr -> Expr + checkExprResolutionId local expr = checkExprResolution local expr expr + + -- error if the given expression resolves globally but not locally + checkExprResolution :: Scopes () -> Expr -> a -> a + checkExprResolution local expr = + if exprResolves global expr && not (anyPrefixResolves local expr) + then scopedError local $ checkErrMsg $ show expr + else id + + -- check if hierarchical prefix of an expr exists in the given scope + anyPrefixResolves :: Scopes () -> Expr -> Bool + anyPrefixResolves local expr = + exprResolves local expr || + case expr of + Dot inner _ -> anyPrefixResolves local inner + Bit inner _ -> anyPrefixResolves local inner + _ -> False + + -- check if expr exists in the given scope + exprResolves :: Scopes a -> Expr -> Bool + exprResolves local (Ident x) = + isJust (lookupElem local x) || isLoopVar local x + exprResolves local expr = + isJust (lookupElem local expr) diff --git a/test/core/interface_check_same.sv b/test/core/interface_check_same.sv new file mode 100644 index 0000000..79b1d6f --- /dev/null +++ b/test/core/interface_check_same.sv @@ -0,0 +1,107 @@ +`define TEST(loc, mod, expr) \ + initial begin \ + $display(`"loc mod.expr %b`", mod.expr); \ + $display(`"loc loc.mod.expr %b`", loc.mod.expr); \ + begin /* exciting shadowing */ \ + localparam i = 1'bx; \ + localparam j = 1'bz; \ + $display(`"loc loc.mod.expr %b`", loc.mod.expr); \ + end \ + end + +`define TEST_FUNC(loc, mod, expr) `TEST(loc, mod, expr()) + +`define TEST_TASK(loc, mod, expr) \ + initial begin \ + $display(`"loc mod.expr():`"); \ + mod.expr(); \ + $display(`"loc loc.mod.expr():`"); \ + loc.mod.expr(); \ + end + +interface Intf; + parameter [3:0] P = 1; + localparam [3:0] L = 2; + wire [3:0] w; + assign w = 3; + modport D(input .x(w + 8'b1)); + function automatic [3:0] F; + return -1; + endfunction + task T; + $display("T called"); + endtask + + if (1) begin : blk + parameter [3:0] P = 4; + localparam [3:0] L = 6; + reg [3:0] w; + initial w = 7; + function automatic [3:0] F; + return 8; + endfunction + task T; + $display("blk.T called"); + endtask + end +endinterface + +module ModAi(Intf i ); + `TEST(ModAi, i, P) + `TEST(ModAi, i, L) + `TEST(ModAi, i, w) + `TEST_FUNC(ModAi, i, F) + `TEST_TASK(ModAi, i, T) + + `TEST(ModAi, i, blk.P) + `TEST(ModAi, i, blk.L) + `TEST(ModAi, i, blk.w) + `TEST_FUNC(ModAi, i, blk.F) + `TEST_TASK(ModAi, i, blk.T) +endmodule + +module ModAj(Intf j); + `TEST(ModAj, j, P) + `TEST(ModAj, j, L) + `TEST(ModAj, j, w) + `TEST_FUNC(ModAj, j, F) + `TEST_TASK(ModAj, j, T) + + `TEST(ModAj, j, blk.P) + `TEST(ModAj, j, blk.L) + `TEST(ModAj, j, blk.w) + `TEST_FUNC(ModAj, j, blk.F) + `TEST_TASK(ModAj, j, blk.T) +endmodule + +module ModBi(Intf.D i); + `TEST(ModBi, i, P) + `TEST(ModBi, i, L) + `TEST(ModBi, i, x) +endmodule + +module ModBj(Intf.D j); + `TEST(ModBj, j, P) + `TEST(ModBj, j, L) + `TEST(ModBj, j, x) +endmodule + +module top; + Intf i(); + ModAi ai(i); + ModAj aj(i); + ModBi bi(i); + ModBj bj(i); + + `TEST(top, i, P) + `TEST(top, i, L) + `TEST(top, i, w) + `TEST_FUNC(top, i, F) + `TEST_TASK(top, i, T) + + `TEST(top, i, blk.P) + `TEST(top, i, blk.L) + `TEST(top, i, blk.w) + `TEST_FUNC(top, i, blk.F) + `TEST_TASK(top, i, blk.T) +endmodule diff --git a/test/core/interface_check_same.v b/test/core/interface_check_same.v new file mode 100644 index 0000000..b4100f8 --- /dev/null +++ b/test/core/interface_check_same.v @@ -0,0 +1,97 @@ +`define TEST(loc, mod, expr) \ + initial begin \ + $display(`"loc mod.expr %b`", i.expr); \ + repeat (2) \ + $display(`"loc loc.mod.expr %b`", top.i.expr); \ + end + +`define TEST_FUNC(loc, mod, expr) \ + initial begin \ + $display(`"loc mod.expr() %b`", i.expr(0)); \ + repeat (2) \ + $display(`"loc loc.mod.expr() %b`", top.i.expr(0)); \ + end + +`define TEST_TASK(loc, mod, expr) \ + initial begin \ + $display(`"loc mod.expr():`"); \ + i.expr(); \ + $display(`"loc loc.mod.expr():`"); \ + top.i.expr(); \ + end + +module top; + if (1) begin : i + localparam [3:0] P = 1; + localparam [3:0] L = 2; + wire [3:0] w; + assign w = 3; + wire [7:0] x; + assign x = w + 8'b1; + function automatic [3:0] F; + input unused; + F = -1; + endfunction + task T; + $display("T called"); + endtask + + if (1) begin : blk + localparam [3:0] P = 4; + localparam [3:0] L = 6; + reg [3:0] w; + initial w = 7; + function automatic [3:0] F; + input unused; + F = 8; + endfunction + task T; + $display("blk.T called"); + endtask + end + end + + `TEST(ModAi, i, P) + `TEST(ModAi, i, L) + `TEST(ModAi, i, w) + `TEST_FUNC(ModAi, i, F) + `TEST_TASK(ModAi, i, T) + + `TEST(ModAi, i, blk.P) + `TEST(ModAi, i, blk.L) + `TEST(ModAi, i, blk.w) + `TEST_FUNC(ModAi, i, blk.F) + `TEST_TASK(ModAi, i, blk.T) + + `TEST(ModAj, j, P) + `TEST(ModAj, j, L) + `TEST(ModAj, j, w) + `TEST_FUNC(ModAj, j, F) + `TEST_TASK(ModAj, j, T) + + `TEST(ModAj, j, blk.P) + `TEST(ModAj, j, blk.L) + `TEST(ModAj, j, blk.w) + `TEST_FUNC(ModAj, j, blk.F) + `TEST_TASK(ModAj, j, blk.T) + + `TEST(ModBi, i, P) + `TEST(ModBi, i, L) + `TEST(ModBi, i, x) + + `TEST(ModBj, j, P) + `TEST(ModBj, j, L) + `TEST(ModBj, j, x) + + `TEST(top, i, P) + `TEST(top, i, L) + `TEST(top, i, w) + `TEST_FUNC(top, i, F) + `TEST_TASK(top, i, T) + + `TEST(top, i, blk.P) + `TEST(top, i, blk.L) + `TEST(top, i, blk.w) + `TEST_FUNC(top, i, blk.F) + `TEST_TASK(top, i, blk.T) +endmodule diff --git a/test/error/interface_bad_expr_arr.sv b/test/error/interface_bad_expr_arr.sv new file mode 100644 index 0000000..f1a0239 --- /dev/null +++ b/test/error/interface_bad_expr_arr.sv @@ -0,0 +1,16 @@ +// pattern: inlining instance "mod" of module "Module" would make expression "a\[0\]\.x" used in "mod" resolvable when it wasn't previously +// location: interface_bad_expr_arr.sv:10:5 +interface InterfaceA; + logic x; +endinterface +interface InterfaceB; + logic x; +endinterface +module Module(InterfaceB b); + assign a[0].x = 1; +endmodule +module top; + InterfaceA a[1](); + InterfaceB b(); + Module mod(b); +endmodule diff --git a/test/error/interface_bad_expr_module.sv b/test/error/interface_bad_expr_module.sv new file mode 100644 index 0000000..ed06e72 --- /dev/null +++ b/test/error/interface_bad_expr_module.sv @@ -0,0 +1,12 @@ +// pattern: inlining instance "mod" of module "Module" would make expression "x" used in "mod" resolvable when it wasn't previously +// location: interface_bad_expr_module.sv:6:5 +interface Interface; +endinterface +module Module(Interface i); + assign x = 1; +endmodule +module top; + wire x; + Interface intf(); + Module mod(intf); +endmodule