From 4b337add8f56ecc42f2f6ce650d4a919c0260d24 Mon Sep 17 00:00:00 2001 From: Moritz Imfeld Date: Tue, 18 Oct 2022 10:47:41 +0200 Subject: [PATCH 1/2] Add check for scalar source operand validity The problem was that Vicuna accepted instructions without checking if the corresponding scalar source registers were valid. Signed-off-by: Moritz Imfeld --- rtl/vproc_core.sv | 39 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/rtl/vproc_core.sv b/rtl/vproc_core.sv index 92f385e..c9d41f4 100644 --- a/rtl/vproc_core.sv +++ b/rtl/vproc_core.sv @@ -286,10 +286,45 @@ module vproc_core import vproc_pkg::*; #( end assign dec_buf_valid_d = (~dec_ready | dec_valid) & ~dec_clear; + // Check if scalar source operands are valid + logic source_xreg_valid; + logic [31:0] instr; + assign instr = xif_issue_if.issue_req.instr; + + always_comb begin + source_xreg_valid = 1'b1; + if (instr[6:0] == 7'h57) begin + unique case (instr[14:12]) + // rs1 must be valid for OPIVX and OPMVX + 3'b100, + 3'b110: begin + source_xreg_valid = xif_issue_if.issue_req.rs_valid[0]; + end + 3'b111: begin + // rs1 must be valid for vsetvli + if (instr[31:30] == 2'b11) begin + source_xreg_valid = xif_issue_if.issue_req.rs_valid[0]; + // rs1 and rs2 must be valid for vsetvl + end else if (instr[31] == 1'b0) begin + source_xreg_valid = xif_issue_if.issue_req.rs_valid[0] & xif_issue_if.issue_req.rs_valid[1]; + end + end + default: source_xreg_valid = 1'b1; + endcase + // rs1 must be valid for all load and store instructions + end else if (instr[6:0] == 7'h27 || instr[6:0] == 7'h07) begin + source_xreg_valid = xif_issue_if.issue_req.rs_valid[0]; + // rs1 and rs2 must be valid for strided load and store instructions + if (instr[27:26] == 2'b10) begin + source_xreg_valid = xif_issue_if.issue_req.rs_valid[0] & xif_issue_if.issue_req.rs_valid[1]; + end + end + end + // Stall instruction offloading in case the instruction ID is already used // by another instruction which is not complete logic instr_valid, issue_id_used; - assign instr_valid = xif_issue_if.issue_valid & ~issue_id_used; + assign instr_valid = xif_issue_if.issue_valid & ~issue_id_used & source_xreg_valid; op_unit instr_unit; op_mode instr_mode; @@ -331,7 +366,7 @@ module vproc_core import vproc_pkg::*; #( // vset[i]vl[i] instruction that will change the configuration in the next // cycle and any subsequent offloaded instruction must be validated w.r.t. // the new configuration. - assign xif_issue_if.issue_ready = dec_ready & ~issue_id_used; + assign xif_issue_if.issue_ready = dec_ready & ~issue_id_used & source_xreg_valid; assign xif_issue_if.issue_resp.accept = dec_valid; assign xif_issue_if.issue_resp.writeback = dec_valid & (((instr_unit == UNIT_ELEM) & instr_mode.elem.xreg) | (instr_unit == UNIT_CFG)); From d58c0bc0dc0ca75ef8e16b1a97405738b3311958 Mon Sep 17 00:00:00 2001 From: Moritz Imfeld Date: Tue, 8 Nov 2022 17:18:36 +0100 Subject: [PATCH 2/2] Move source_reg_valid logic from core to decoder To avoid code duplication, the case statement in the core module has been removed. Now the case statement in the decoder is reused to decide whether a scalar source register is used for a particular instruction. To pass the signals from the decoder to the core module, I added a signal called xreg to the op_regs typedef. The vreg signal in the op_regs typedef could not be reused because it is also 0 if rs2 is not used at all or if rs2 is an immediate. Signed-off-by: Moritz Imfeld --- rtl/vproc_core.sv | 33 +-------------------------------- rtl/vproc_decoder.sv | 13 +++++++++++-- rtl/vproc_pkg.sv | 1 + 3 files changed, 13 insertions(+), 34 deletions(-) diff --git a/rtl/vproc_core.sv b/rtl/vproc_core.sv index c9d41f4..b7a713f 100644 --- a/rtl/vproc_core.sv +++ b/rtl/vproc_core.sv @@ -288,38 +288,7 @@ module vproc_core import vproc_pkg::*; #( // Check if scalar source operands are valid logic source_xreg_valid; - logic [31:0] instr; - assign instr = xif_issue_if.issue_req.instr; - - always_comb begin - source_xreg_valid = 1'b1; - if (instr[6:0] == 7'h57) begin - unique case (instr[14:12]) - // rs1 must be valid for OPIVX and OPMVX - 3'b100, - 3'b110: begin - source_xreg_valid = xif_issue_if.issue_req.rs_valid[0]; - end - 3'b111: begin - // rs1 must be valid for vsetvli - if (instr[31:30] == 2'b11) begin - source_xreg_valid = xif_issue_if.issue_req.rs_valid[0]; - // rs1 and rs2 must be valid for vsetvl - end else if (instr[31] == 1'b0) begin - source_xreg_valid = xif_issue_if.issue_req.rs_valid[0] & xif_issue_if.issue_req.rs_valid[1]; - end - end - default: source_xreg_valid = 1'b1; - endcase - // rs1 must be valid for all load and store instructions - end else if (instr[6:0] == 7'h27 || instr[6:0] == 7'h07) begin - source_xreg_valid = xif_issue_if.issue_req.rs_valid[0]; - // rs1 and rs2 must be valid for strided load and store instructions - if (instr[27:26] == 2'b10) begin - source_xreg_valid = xif_issue_if.issue_req.rs_valid[0] & xif_issue_if.issue_req.rs_valid[1]; - end - end - end + assign source_xreg_valid = (!dec_data_d.rs1.xreg | xif_issue_if.issue_req.rs_valid[0]) & (!dec_data_d.rs2.xreg | xif_issue_if.issue_req.rs_valid[1]); // Stall instruction offloading in case the instruction ID is already used // by another instruction which is not complete diff --git a/rtl/vproc_decoder.sv b/rtl/vproc_decoder.sv index 0bd34f1..f0044b6 100644 --- a/rtl/vproc_decoder.sv +++ b/rtl/vproc_decoder.sv @@ -61,10 +61,12 @@ module vproc_decoder #( mode_o.unused = DONT_CARE_ZERO ? '0 : 'x; rs1_o.vreg = DONT_CARE_ZERO ? 1'b0 : 1'bx; + rs1_o.xreg = 1'b0; rs1_o.r.xval = DONT_CARE_ZERO ? '0 : 'x; rs1_o.r.vaddr = DONT_CARE_ZERO ? '0 : 'x; rs2_o.vreg = DONT_CARE_ZERO ? 1'b0 : 1'bx; + rs2_o.xreg = 1'b0; rs2_o.r.xval = DONT_CARE_ZERO ? '0 : 'x; rs2_o.r.vaddr = DONT_CARE_ZERO ? '0 : 'x; @@ -151,6 +153,7 @@ module vproc_decoder #( mode_o.lsu.nfields = instr_i[31:29]; rs1_o.vreg = 1'b0; // rs1 is an x register + rs1_o.xreg = 1'b1; rs1_o.r.xval = x_rs1_i; rd_o.vreg = 1'b1; // vd/vs3 is a vreg @@ -228,6 +231,7 @@ module vproc_decoder #( 2'b10: begin // strided load/store mode_o.lsu.stride = LSU_STRIDED; rs2_o.vreg = 1'b0; + rs2_o.xreg = 1'b1; rs2_o.r.xval = x_rs2_i; end 2'b01, @@ -254,12 +258,14 @@ module vproc_decoder #( 3'b001, // OPFVV 3'b010: begin // OPMVV rs1_o.vreg = 1'b1; // rs1 is a vector register + rs1_o.xreg = 1'b0; rs1_o.r.vaddr = instr_vs1; rs2_o.vreg = 1'b1; // rs2 is a vector register rs2_o.r.vaddr = instr_vs2; end 3'b011: begin // OPIVI rs1_o.vreg = 1'b0; // rs1 field contains immediate (sign extend for all except slide instructions) + rs1_o.xreg = 1'b0; rs1_o.r.xval = ((instr_i[31:26] == 6'b001110) | (instr_i[31:26] == 6'b001111)) ? {{27{1'b0}}, instr_vs1} : {{27{instr_vs1[4]}}, instr_vs1}; rs2_o.vreg = 1'b1; // rs2 is a vector register rs2_o.r.vaddr = instr_vs2; @@ -267,15 +273,18 @@ module vproc_decoder #( 3'b100, // OPIVX 3'b110: begin // OPMVX rs1_o.vreg = 1'b0; // rs1 is an x register + rs1_o.xreg = 1'b1; rs1_o.r.xval = x_rs1_i; rs2_o.vreg = 1'b1; // rs2 is a vector register rs2_o.r.vaddr = instr_vs2; end 3'b111: begin // OPCFG rs1_o.vreg = 1'b0; // rs1 is either x reg or immediate - rs1_o.r.xval = (instr_i[31:30] != 2'b11) ? x_rs1_i : {{27{1'b0}}, instr_vs1}; + rs1_o.xreg = instr_i[31:30] != 2'b11; + rs1_o.r.xval = rs1_o.xreg ? x_rs1_i : {{27{1'b0}}, instr_vs1}; rs2_o.vreg = 1'b0; // rs2 is either x reg or immediate - rs2_o.r.xval = (instr_i[31:30] == 2'b10) ? x_rs2_i : {{21{1'b0}}, instr_i[30] & ~instr_i[31], instr_i[29:20]}; + rs2_o.xreg = instr_i[31:30] == 2'b10; + rs2_o.r.xval = rs2_o.xreg ? x_rs2_i : {{21{1'b0}}, instr_i[30] & ~instr_i[31], instr_i[29:20]}; end default: ; endcase diff --git a/rtl/vproc_pkg.sv b/rtl/vproc_pkg.sv index b84f18d..f448e77 100644 --- a/rtl/vproc_pkg.sv +++ b/rtl/vproc_pkg.sv @@ -298,6 +298,7 @@ typedef struct packed { // source register type: typedef struct packed { logic vreg; + logic xreg; `ifdef VPROC_OP_REGS_UNION union { `else