Skip to content

Commit 8811ecc

Browse files
committed
fix(simulator): a wide unbased-literal ('1) ternary branch builds a wide pointer at max(expr_context.width, width), but builds_wide_pointer and the cranelift to_wide_ptr keyed on its 0-width field and dropped the branch's high words
1 parent bd9605c commit 8811ecc

3 files changed

Lines changed: 140 additions & 3 deletions

File tree

crates/simulator/src/backend/cranelift/expression.rs

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2998,8 +2998,14 @@ impl ProtoExpression {
29982998
if returns_wide_pointer(expr) {
29992999
// Widen a short branch to `nb` before `emit_wide_select`
30003000
// strides it (mixed-width case arms: a 136-bit branch → a
3001-
// 256-bit dst).
3002-
widen_wide_ptr(builder, val, calc_native_bytes(expr.width()), nb)
3001+
// 256-bit dst). Size by `materialized_width`, not `width()`,
3002+
// which is 0 for a sentinel branch (see `materialized_width`).
3003+
widen_wide_ptr(
3004+
builder,
3005+
val,
3006+
calc_native_bytes(expr.materialized_width()),
3007+
nb,
3008+
)
30033009
} else {
30043010
let slot = alloc_wide_zero(builder, nb);
30053011
builder.ins().store(MemFlagsData::trusted(), val, slot, 0);

crates/simulator/src/ir/expression.rs

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -777,6 +777,23 @@ impl ProtoExpression {
777777
}
778778
}
779779

780+
/// The width at which `build_binary` materializes this node — same as
781+
/// [`width`](Self::width) except for the unsized all-bit sentinel
782+
/// (`'0`/`'1`/`'x`/`'z`, a `Value::U64` with width 0), which fills
783+
/// `max(expr_context.width, width)`. Consumers that stride the materialized
784+
/// buffer (`builds_wide_pointer`, the wide-ternary `to_wide_ptr`) must key on
785+
/// this; the sentinel's raw `width` is 0 and would truncate its high words.
786+
pub fn materialized_width(&self) -> usize {
787+
match self {
788+
ProtoExpression::Value {
789+
value: Value::U64(ValueU64 { width: 0, .. }),
790+
width,
791+
expr_context,
792+
} => (*width).max(expr_context.width),
793+
_ => self.width(),
794+
}
795+
}
796+
780797
/// Returns a guaranteed upper bound on the number of significant bits
781798
/// in the Cranelift value produced by build_binary().
782799
/// Used to skip redundant truncation masks at store time.
@@ -885,7 +902,9 @@ impl ProtoExpression {
885902
*var_full_width > W
886903
&& !(select.is_some() && dynamic_select.is_none() && *width <= 64)
887904
}
888-
ProtoExpression::Value { width, .. } => *width > W,
905+
// The all-bit sentinel materializes wider than its 0 `width` field
906+
// (see `materialized_width`); key on that.
907+
ProtoExpression::Value { .. } => self.materialized_width() > W,
889908
ProtoExpression::Concatenation { width, .. } => *width > W,
890909
ProtoExpression::Ternary {
891910
width,

crates/simulator/src/tests/simulation.rs

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1465,6 +1465,118 @@ fn wide_dual_slot_dyn_array_rmw_writeloss() {
14651465
}
14661466
}
14671467

1468+
#[test]
1469+
fn wide_masked_rmw_allones_literal_mask() {
1470+
// The install path of the byte-write-enable array uses mask = the all-ones
1471+
// literal `'1` (via `if sel ? '1 : partial`), so `arr[idx] = (arr[idx] & ~'1)
1472+
// | (din & '1)` must be a FULL overwrite. If `'1` is mis-sized in a wide
1473+
// context, `~'1` is not all-zeros and the install leaks old bits.
1474+
let code = r#"
1475+
module Top (
1476+
clk: input clock,
1477+
we: input logic,
1478+
sel: input logic,
1479+
idx: input logic<4>,
1480+
din: input logic<512>,
1481+
msk: input logic<512>,
1482+
dout: output logic<512>,
1483+
) {
1484+
var arr: logic<512> [16];
1485+
let m: logic<512> = if sel ? '1 : msk;
1486+
always_ff {
1487+
if we {
1488+
arr[idx] = (arr[idx] & ~m) | (din & m);
1489+
}
1490+
}
1491+
assign dout = arr[idx];
1492+
}
1493+
"#;
1494+
let all_ones: num_bigint::BigUint =
1495+
(num_bigint::BigUint::from(1u8) << 512usize) - num_bigint::BigUint::from(1u8);
1496+
let d0: num_bigint::BigUint = (num_bigint::BigUint::from(1u8) << 511usize)
1497+
| (num_bigint::BigUint::from(1u8) << 200usize)
1498+
| num_bigint::BigUint::from(0xDEADu32);
1499+
let d1: num_bigint::BigUint =
1500+
(num_bigint::BigUint::from(1u8) << 400usize) | num_bigint::BigUint::from(0xBEEFu32);
1501+
for config in Config::all().into_iter().filter(|c| !c.use_4state) {
1502+
dbg!(&config);
1503+
let ir = analyze(code, &config);
1504+
let mut sim = Simulator::new(ir, None);
1505+
let clk = sim.get_clock("clk").unwrap();
1506+
sim.set("idx", Value::new(5, 4, false));
1507+
sim.set("we", Value::new(1, 1, false));
1508+
// Seed arr[5] = d0 with a real (partial-shaped but full) mask.
1509+
sim.set("sel", Value::new(0, 1, false));
1510+
sim.set("msk", Value::new_biguint(all_ones.clone(), 512, false));
1511+
sim.set("din", Value::new_biguint(d0.clone(), 512, false));
1512+
sim.step(&clk);
1513+
// Install-style full overwrite via mask = '1.
1514+
sim.set("sel", Value::new(1, 1, false));
1515+
sim.set("din", Value::new_biguint(d1.clone(), 512, false));
1516+
sim.step(&clk);
1517+
sim.set("we", Value::new(0, 1, false));
1518+
sim.step(&clk);
1519+
assert_eq!(
1520+
sim.get("dout").unwrap(),
1521+
Value::new_biguint(d1.clone(), 512, false),
1522+
"'1 mask install leaked old bits (not a full overwrite) ({config:?})"
1523+
);
1524+
}
1525+
}
1526+
1527+
#[test]
1528+
fn wide_unbased_all_ones_ternary_branch() {
1529+
// Regression: a wide (>128-bit) unbased all-ones literal `'1` as a TERNARY
1530+
// BRANCH. The JIT/AOT-C `build_binary` materializes the all-bit sentinel at
1531+
// `max(expr_context.width, width)` (a wide pointer here), but the
1532+
// `builds_wide_pointer` predicate keyed only on the Value's `width` field
1533+
// (0 for an unsized literal) said "scalar" — so `to_wide_ptr` stored the wide
1534+
// value as a scalar and dropped its high words, yielding a partial `'1`
1535+
// (low bits set, high bits zero). Direct `'1` and `~'1` were fine; only the
1536+
// ternary branch tripped the mismatch. Cranelift and cc both consult the
1537+
// predicate, so both were wrong; the interpreter was correct.
1538+
let code = r#"
1539+
module Top (
1540+
sel: input logic,
1541+
a: input logic<512>,
1542+
direct: output logic<512>,
1543+
tern: output logic<512>,
1544+
notd: output logic<512>,
1545+
) {
1546+
let md: logic<512> = '1;
1547+
let mt: logic<512> = if sel ? '1 : a;
1548+
assign direct = md;
1549+
assign tern = mt;
1550+
assign notd = ~md;
1551+
}
1552+
"#;
1553+
let all_ones: num_bigint::BigUint =
1554+
(num_bigint::BigUint::from(1u8) << 512usize) - num_bigint::BigUint::from(1u8);
1555+
for config in Config::all().into_iter().filter(|c| !c.use_4state) {
1556+
dbg!(&config);
1557+
let ir = analyze(code, &config);
1558+
let mut sim = Simulator::new(ir, None);
1559+
sim.set("sel", Value::new(1, 1, false));
1560+
sim.set("a", Value::new(0, 512, false));
1561+
sim.step(&Event::Clock(VarId::SYNTHETIC));
1562+
assert_eq!(
1563+
sim.get("tern").unwrap(),
1564+
Value::new_biguint(all_ones.clone(), 512, false),
1565+
"wide `'1` ternary branch lost its high words ({config:?})"
1566+
);
1567+
assert_eq!(
1568+
sim.get("direct").unwrap(),
1569+
Value::new_biguint(all_ones.clone(), 512, false),
1570+
"{config:?}"
1571+
);
1572+
assert_eq!(
1573+
sim.get("notd").unwrap(),
1574+
Value::new(0, 512, false),
1575+
"{config:?}"
1576+
);
1577+
}
1578+
}
1579+
14681580
#[test]
14691581
fn nested_array_index_const_array() {
14701582
// Regression for a nested array index `mem[A[idx]]` with a const array

0 commit comments

Comments
 (0)