celledges: support shift ops

[nakengelhardt]

No description provided.

[povik]

Wait, I am not sure this handles Y_WIDTH != WIDTH, let me double-check.

[povik]

Yeah, looks like the case of Y_WIDTH != WIDTH is not handled properly.

[nakengelhardt]

Oh right, should that be int width = min(GetSize(cell->getPort(ID::A)), GetSize(cell->getPort(ID::Y)));?

[povik]

I don't think so since then you don't register the dependencies of the higher bits on Y.

[nakengelhardt]

Had to take a moment and draw my diagrams again, but I think this should do it.

[povik]

I think we have yet to take into account the sign-extension of the input, e.g. with a $sshr cell with signed A input of width smaller than the output, the top Y bit will depend on the top A bit, though this code won't register it.

[nakengelhardt]

Right, that now is a problem when previously I thought it was covered since the msb of A could already end up in any (lower) position. Thanks for paying attention to that!

[povik]

Looking at how tricky this is to get right I started writing a pass to verify the celledges data with a SAT solver. Let's see, let me try it out on the code from the PR.

[nakengelhardt]

Isn't that what test_cells is supposed to do with this? (I ran it quickly on $sshr after the last commit before going off to dinner, was going to look into what it actually does later)

[povik]

min(i, a_width - 1) here I think

[povik]

Isn't that what test_cells is supposed to do with this?

I don't think so. Or rather, I am letting test_cell enumerate the different cell operand shapes and then let the new pass do the work of verifying the bit-level influences among the ports, but maybe there's an existing way I don't know about.

[povik]

It found a failing test case on $shr, but I have yet to write the part of the pass that prints out the failing model.

Looking at the const_shr implementation

RTLIL::Const RTLIL::const_shr(const RTLIL::Const &arg1, const RTLIL::Const &arg2, bool signed1, bool, int result_len)
{
	RTLIL::Const arg1_ext = arg1;
	extend_u0(arg1_ext, max(result_len, GetSize(arg1)), signed1);
	return const_shift_worker(arg1_ext, arg2, false, false, +1, result_len);
}

the signed extension of arg1 up to result_len might be what's throwing the celledges data off.

[povik]

Isn't that what test_cells is supposed to do with this?

Ah, you are right, that's exactly what test_cell -edges does. Should have grepped the source for celledges before I started writing something new.

[povik]

So test_cell -edges is pretty strict. It doesn't let us be conservative about the edges and leave extra edges on input-output bit pairs if there's not an actual way to flip the output with that input on the specific cell configuration. With shift cells we would need to reason about the size of the B operand to pass the test_cell -edges test. It's something to consider whether we don't want to relax the test_cell -edges test, so that it only requires the edges data to be a cover of the actual edges, not necessarily equal to it.

[povik]

At least one unhandled case I see in the code now is the distinction between signed and unsigned B in case of $shift or $shiftx cell.

[nakengelhardt]

Oh yeah I haven't started $shift/$shiftx at all yet, was still stuck with $shr ($sshr also not tested yet)

[povik]

OK, will focus on shr first then

[povik]

With the latest commit test_cell -edges $shr $sshr passes, but I suspect the test considers x-bits equal to zero bits, which I think we should change.

[povik]

but I suspect the test considers x-bits equal to zero bits, which I think we should change.

OK, I understood the definition of $shr wrong (I thought it pads with x-bits, but beyond Y_WIDTH it zero-pads). So this x-bit point is moot.

[nakengelhardt]

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.

[povik]

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.

[nakengelhardt]

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!

[povik]

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.

[nakengelhardt]

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?)

[jix]

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.

[nakengelhardt]

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

[whitequark]

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.

[nakengelhardt]

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.

[whitequark]

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.

[nakengelhardt]

LGTM