[libre-riscv-dev] [Bug 64] data handling / io control / data routing API needed

[bugzilla-daemon at libre-riscv.org]

http://bugs.libre-riscv.org/show_bug.cgi?id=64

--- Comment #44 from Luke Kenneth Casson Leighton <lkcl at lkcl.net> ---
(In reply to Jacob Lifshay from comment #40)
> (In reply to Luke Kenneth Casson Leighton from comment #23)
> > (In reply to Jacob Lifshay from comment #8)
> > > > * Signal(1, reset=0), both width 1 and reset=0 are the defaults and may
> > > >   be removed
> > > I'd like reset=0 to be left as it makes it explicit that the reset value
> > > actually matters whereas most other signals the reset value doesn't really
> > > matter.
> > > so Signal(reset=0)
> > 
> >  ok - yes, the usual convention there is to add a comment, so that people
> >  are not caught out by surprise at the explicit mention of something that's
> >  known to be a default value.
> > 
> >  which is ok with me, if you feel it helps readability, to do that.
> > 
> >  however... this masks something that's really important: if the reset value
> >  doesn't matter, it's *really important* to put "reset_less=True" to not only
> >  make that absolutely clear, but to *STOP* nmigen from generating unnecessary
> >  gates that the optimiser will in *no way* be able to spot.
> > 
> >  which is why i mentioned in an earlier comment that reset_less=True should,
> >  really, be the preferred... what's the word... "pattern" / "distinguisher".
> I didn't use reset_less=True because that way we can prevent "optimizations"
> that depend on the values being X and going to town removing critical logic.

 it doesn't work that way.  all that happens is, at the top of the
 IR output, an assignment to the reset value is left out.

 if reset_less=False, that assignment is near-guaranteed (there's apparently
 some guesses made at situations where a reset is *not* needed)

 whereas if reset_less=True, those guesses are DISABLED, and the reset is
 GUARANTEED not to be outputted into the IR/verilog.

 so it's the other way round: setting reset_less=True GUARANTEES that the
 clock-based clk/rst code will NOT be generated.

 which is exactly what is needed when you KNOW that you are using a Signal
 that you do NOT want clk/rst code to be generated.



> If nmigen can somehow guarantee that those undesired optimizations aren't
> occurring, then I agree that reset_less=True should be the default.

 that's not nmigen's responsibility, it's *our* responsibility.
 it can't be.  *we* need to be the ones that make the declaration,
 "this variable is an intermediary as part of a combinatorial
  block, no state or reset is required".

 that's precisely what reset_less=True is for, and there is absolutely
 no way for nmigen to correctly - and safely - determine that.

 (note: the OTHER WAY ROUND however - when reset_less=False - *IS*
  sometimes possible).


 it requires careful case-by-case analysis, and is why i am doing
 the graphviz analysis on every single module.  really, though, we
 do need formal proofs.

 nmigen takes the "safe" route by making reset_less=False the default
 because most novice hardware engineers get this hopelessly wrong,
 forgetting to put in a reset and costing hundreds to millions of
 dollars in NREs, through one simple mistake.

 basically, reset_less=True needs to be given the same fundamental
 level of priority as something as simple as "getting the bitwidth
 right" or "getting the algorithm right"


> >  in addition, one thing that i've found you really *really* have to watch
> >  out for: if the width is not equal to 1, sometimes setting an integer
> >  constant is NOT ACCURATE.
> that's an nmigen bug then. If you find any particular cases, report them to
> the nmigen project.

 i can't.  they're forcing the requirement onto all people wishing to
 interact with them to have a github account.

-- 
You are receiving this mail because:
You are on the CC list for the bug.