Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

celledges: support shift ops #3972

Merged
merged 8 commits into from
Mar 8, 2024
Merged

celledges: support shift ops #3972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d0e559a
celledges: support shift ops
nakengelhardt Oct 2, 2023
2d6d6a8
fix handling a_width != y_width
nakengelhardt Oct 2, 2023
6c562c7
fix handling right shifts
nakengelhardt Oct 2, 2023
bdd74e6
celledges: Account for shift down of x-bits wrt B port
povik Oct 16, 2023
4cce491
celledges: s/x_jump/zpad_jump/
povik Dec 9, 2023
a96c257
celledges: Add messy rules that do pass the tests
povik Dec 11, 2023
134eb15
celledges: Clean up shift rules
povik Jan 19, 2024
cb86efa
celledges: Add test of shift cells edge data
povik Jan 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
157 changes: 150 additions & 7 deletions kernel/celledges.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,141 @@ void demux_op(AbstractCellEdgesDatabase *db, RTLIL::Cell *cell)
}
}

void shift_op(AbstractCellEdgesDatabase *db, RTLIL::Cell *cell)
{
bool is_signed = cell->getParam(ID::A_SIGNED).as_bool();
bool is_b_signed = cell->getParam(ID::B_SIGNED).as_bool();
int a_width = GetSize(cell->getPort(ID::A));
int b_width = GetSize(cell->getPort(ID::B));
int y_width = GetSize(cell->getPort(ID::Y));

// Behavior of the different shift cells:
//
// $shl, $sshl -- shifts left by the amount on B port, B always unsigned
// $shr, $sshr -- ditto right
// $shift, $shiftx -- shifts right by the amount on B port, B optionally signed
//
// Sign extension (if A signed):
//
// $shl, $shr, $shift -- only sign-extends up to size of Y, then shifts in zeroes
// $sshl, $sshr -- fully sign-extends
// $shiftx -- no sign extension
//
// Because $shl, $sshl only shift left, and $shl sign-extens up to size of Y, they
// are effectively the same.

// the cap below makes sure we don't overflow in the arithmetic further down, though
// it makes the edge data invalid once a_width approaches the order of 2**30
// (that ever happening is considered improbable)
int b_width_capped = min(b_width, 30);

int b_high, b_low;
if (!is_b_signed) {
b_high = (1 << b_width_capped) - 1;
b_low = 0;
} else {
b_high = (1 << (b_width_capped - 1)) - 1;
b_low = -(1 << (b_width_capped - 1));
}

for (int i = 0; i < y_width; i++){
// highest position of Y that can change with the value of B
int b_range_upper = 0;
// 1 + highest position of A that can be moved to Y[i]
int a_range_upper;
// lowest position of A that can be moved to Y[i]
int a_range_lower;

if (cell->type.in(ID($shl), ID($sshl))) {
b_range_upper = a_width + b_high;
if (is_signed) b_range_upper -= 1;
a_range_lower = max(0, i - b_high);
a_range_upper = min(i+1, a_width);
} else if (cell->type.in(ID($shr), ID($sshr)) || (cell->type.in(ID($shift), ID($shiftx)) && !is_b_signed)) {
b_range_upper = a_width;
a_range_lower = min(i, a_width - 1);
a_range_upper = min(i+1 + b_high, a_width);
} else if (cell->type.in(ID($shift), ID($shiftx)) && is_b_signed) {
// can go both ways depending on sign of B
// 2's complement range is different depending on direction
b_range_upper = a_width - b_low;
a_range_lower = max(0, i + b_low);
if (is_signed)
a_range_lower = min(a_range_lower, a_width - 1);
a_range_upper = min(i+1 + b_high, a_width);
} else {
log_assert(false && "unreachable");
}

if (i < b_range_upper) {
for (int k = a_range_lower; k < a_range_upper; k++)
db->add_edge(cell, ID::A, k, ID::Y, i, -1);
} else {
// only influence is through sign extension
if (is_signed)
db->add_edge(cell, ID::A, a_width - 1, ID::Y, i, -1);
}

for (int k = 0; k < b_width; k++) {
// left shifts
if (cell->type.in(ID($shl), ID($sshl))) {
if (a_width == 1 && is_signed) {
int skip = 1 << (k + 1);
int base = skip -1;
if (i % skip != base && i - a_width + 2 < 1 << b_width)
db->add_edge(cell, ID::B, k, ID::Y, i, -1);
} else if (is_signed) {
if (i - a_width + 2 < 1 << b_width)
db->add_edge(cell, ID::B, k, ID::Y, i, -1);
} else {
if (i - a_width + 1 < 1 << b_width)
db->add_edge(cell, ID::B, k, ID::Y, i, -1);
}
// right shifts
} else if (cell->type.in(ID($shr), ID($sshr)) || (cell->type.in(ID($shift), ID($shiftx)) && !is_b_signed)) {
if (is_signed) {
bool shift_in_bulk = i < a_width - 1;
// can we jump into the zero-padding by toggling B[k]?
bool zpad_jump = (((y_width - i) & ((1 << (k + 1)) - 1)) != 0 \
&& (((y_width - i) & ~(1 << k)) < (1 << b_width)));

if (shift_in_bulk || (cell->type.in(ID($shr), ID($shift), ID($shiftx)) && zpad_jump))
db->add_edge(cell, ID::B, k, ID::Y, i, -1);
} else {
if (i < a_width)
db->add_edge(cell, ID::B, k, ID::Y, i, -1);
}
// bidirectional shifts (positive B shifts right, negative left)
} else if (cell->type.in(ID($shift), ID($shiftx)) && is_b_signed) {
if (is_signed) {
if (k != b_width - 1) {
bool r_shift_in_bulk = i < a_width - 1;
// assuming B is positive, can we jump into the upper zero-padding by toggling B[k]?
bool r_zpad_jump = (((y_width - i) & ((1 << (k + 1)) - 1)) != 0 \
&& (((y_width - i) & ~(1 << k)) <= b_high));
// assuming B is negative, can we influence Y[i] by toggling B[k]?
bool l = a_width - 2 - i >= b_low;
if (a_width == 1) {
// in case of a_width==1 we go into more detailed reasoning
l = l && (~(i - a_width) & ((1 << (k + 1)) - 1)) != 0;
}
if (r_shift_in_bulk || r_zpad_jump || l)
db->add_edge(cell, ID::B, k, ID::Y, i, -1);
} else {
if (y_width - i <= b_high || a_width - 2 - i >= b_low)
db->add_edge(cell, ID::B, k, ID::Y, i, -1);
}
} else {
if (a_width - 1 - i >= b_low)
db->add_edge(cell, ID::B, k, ID::Y, i, -1);
}
} else {
log_assert(false && "unreachable");
}
}
}
}

PRIVATE_NAMESPACE_END

bool YOSYS_NAMESPACE_PREFIX AbstractCellEdgesDatabase::add_edges_from_cell(RTLIL::Cell *cell)
Expand Down Expand Up @@ -201,11 +336,10 @@ bool YOSYS_NAMESPACE_PREFIX AbstractCellEdgesDatabase::add_edges_from_cell(RTLIL
return true;
}

// FIXME:
// if (cell->type.in(ID($shl), ID($shr), ID($sshl), ID($sshr), ID($shift), ID($shiftx))) {
// shift_op(this, cell);
// return true;
// }
if (cell->type.in(ID($shl), ID($shr), ID($sshl), ID($sshr), ID($shift), ID($shiftx))) {
shift_op(this, cell);
return true;
}

if (cell->type.in(ID($lt), ID($le), ID($eq), ID($ne), ID($eqx), ID($nex), ID($ge), ID($gt))) {
compare_op(this, cell);
Expand All @@ -227,8 +361,17 @@ bool YOSYS_NAMESPACE_PREFIX AbstractCellEdgesDatabase::add_edges_from_cell(RTLIL
return true;
}

// FIXME: $mul $div $mod $divfloor $modfloor $slice $concat
// FIXME: $lut $sop $alu $lcu $macc $fa
// FIXME: $mul $div $mod $divfloor $modfloor $pow $slice $concat $bweqx
// FIXME: $lut $sop $alu $lcu $macc $fa $logic_and $logic_or $bwmux

// FIXME: $_BUF_ $_NOT_ $_AND_ $_NAND_ $_OR_ $_NOR_ $_XOR_ $_XNOR_ $_ANDNOT_ $_ORNOT_
// FIXME: $_MUX_ $_NMUX_ $_MUX4_ $_MUX8_ $_MUX16_ $_AOI3_ $_OAI3_ $_AOI4_ $_OAI4_

// FIXME: $specify2 $specify3 $specrule ???
// FIXME: $equiv $set_tag $get_tag $overwrite_tag $original_tag

if (cell->type.in(ID($assert), ID($assume), ID($live), ID($fair), ID($cover), ID($initstate), ID($anyconst), ID($anyseq), ID($allconst), ID($allseq)))
return true; // no-op: these have either no inputs or no outputs

return false;
}
Expand Down
4 changes: 2 additions & 2 deletions passes/sat/eval.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,13 +543,13 @@ struct EvalPass : public Pass {

int pos = 0;
for (auto &c : tabsigs.chunks()) {
tab_line.push_back(log_signal(RTLIL::SigSpec(tabvals).extract(pos, c.width)));
tab_line.push_back(log_signal(RTLIL::SigSpec(tabvals).extract(pos, c.width), false));
pos += c.width;
}

pos = 0;
for (auto &c : signal.chunks()) {
tab_line.push_back(log_signal(value.extract(pos, c.width)));
tab_line.push_back(log_signal(value.extract(pos, c.width), false));
pos += c.width;
}

Expand Down
1 change: 1 addition & 0 deletions tests/various/celledges_shift.ys
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
test_cell -s 1705659041 -n 100 -edges $shift $shiftx $shl $shr $sshl $sshr
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder, should we leave the seed unspecified in order to get better test coverage? It still prints the chosen seed in the log so we can reproduce if necessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the potential for confusion when a CI job starts failing for reasons unrelated to the underlying code change is much worse than the benefit of the extra coverage we would get. FWIW I let the test_cell command complete with a much larger N when I was working on this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the alternative to failing on a later CI run from a previous breaking change is to never fail, which means we don't realize that it's broken at all. I would quibble with calling that a "much better" outcome!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess where I see the value is that we can discount "this is broken due to an unlucky seed" when reasoning about CI failures, which I weight against the chance that this will help us find a bug which would otherwise stay hidden, which I think is low. (Anyone touching the shift edges code should be asked to do a long test_cell run anyway.)

If someone down the line touches e.g. the SAT code and this prompts a failure in this test, we will know it's the SAT changes for sure, and that's valuable. At least that's how I think about this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see your point but I don't think it would be that difficult to trace which part is the problem. If the test fails I think the first debugging step would be to run the same test with the code before the change, at which point it would become obvious that the failure existed earlier. I guess that does require knowing that this test uses random generation and that you need to use the same seed, which you might miss if you don't read the log backscroll far enough. (Maybe for convenience test_cell should print the seed again as the last line if a test fails?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer CI runs triggered by PRs to be deterministic, but I don't see an issue with using randomized seeds for the daily CI runs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's worth adding lines in the CI configurations for this one test, let's just keep it deterministic then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would quibble with calling that a "much better" outcome!

I strongly disagree with any argument that describes nondeterministic CI a non-worse outcome. If your CI is nondeterministic, it reduces faith in that the CI works at all. Eventually you create a culture of simply restarting the CI run if something fails because it's known to be flaky.

It's OK to have a separate job that is clearly and expressly marked as nondeterministic (this is basically a simple fuzzer) but merging must not be gated on it unless there's strong explicit consensus that this is a good idea.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But by that argument, it would be "better" if we were never running any synth pass in CI at all, since abc is currently nondeterministically breaking CI.

Copy link
Member

@whitequark whitequark Mar 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I think not running nondeterministic tests at all is better than nondeterministically breaking CI. If we (YosysHQ) took testing seriously we would have made ABC deterministic, which I have offered workable solutions for.

I will note that I've also introduced the -noabc flow, which could be an interim alternative to using ABC while it's still nondeterministic.

Loading