[libre-riscv-dev] [Bug 120] implement RISC-V FSGNJ instruction

[bugzilla-daemon at libre-riscv.org]

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

--- Comment #23 from Michael Nolan <mtnolan2640 at gmail.com> ---
> okaaay, so this gets interesting.  take a look at the second
> screenshot (after doing a git pull and re-run the unit tests
> you can generate it yourself if you prefer).
>
> http://bugs.libre-riscv.org/attachment.cgi?id=20
>
> can you see that there's some spurious inexplicable maths
> going on, there, compared to the first graphviz?
>
> http://bugs.libre-riscv.org/attachment.cgi?id=19
>
> that extra "stuff" is because inside the FPNumDecode, for
> convenience i got it to subtract the "offset" that's part
> of an exponent, and the "create()" function adds it back on.
>
> so if you do yosys "show sc_decode_a" you'll find a whole stack
> of extra stuff, none of which is actually necessary.
>
> we know it's not necessary because the previous version - without
> FPNumDecode - worked perfectly well and pass all unit tests.
>
> this is why i emphasise in https://libre-riscv.org/HDL_workflow/
> that it's essential to run the yosys "show" command as part of the
> workflow.  you get to see the actual graph - the actual gates
> created - and go "oink?? what's that doing in there??"

I'll admit, I did run the yosys show, but was only looking at the Mux
part, and didn't notice that extra add.

I'm a little surprised that part doesn't get optimized away (I played
around with flattening fsignj and running yosys' optimizations on
it). I
wonder if it's because FPNumDecode adds some extra bits to the
exponant
when it decodes it.


>
> second (minor) thing, you'd created a Signal "sign" and then had
> immediately overwritten it with a python variable *also* named
> "sign" which was the result of the Mux().
>
> this seems to be a common source of confusion in nmigen, i've seen
> this done before: a python variable is created and then overwritten.
> remember that nmigen creates ASTs (abstract syntax trees) and so
> anything added to m.d.comb (or m.d.sync) is completely unused.

This was why I was looking at the graph to see if the mux was actually
there. Though it and
https://git.libre-riscv.org/?p=ieee754fpu.git;a=blob;f=src/ieee754/fpmul/specialcases.py;h=066d2b9f0fc15dd153cb1c7b262901636b7fe344;hb=HEAD
(line 88) seem to work ok, is that because the result variable is
added to the nmigen ast in the "m.d.comb +="


> third thing: again, which i spotted only after looking at the
> graphviz: Mux(opcode[0], ~b.s, b.s) was extremely puzzling
> in the graph however is quite logical in the code.
>
> i thought about it and realised that the Mux could be replaced with
> an XOR gate.  sign = opcode[0] ^ ~b.s
>
> however that's really not very obvious so i provided a (terse)
> explanatory comment.

Note, this should probably read (and does in fsgnj.py) as `sign =
opcode[0] ^ b.s`

This doesn't seem any more obvious in the graph to me than the Mux and
invert so idk.

> i'd suggest getting rid of FPNumDecode, and returning to b[-1] etc.
> can i leave that with you?

Sure 


Edit: Whoops, looks like I replied to the mailing list post, not the bugzilla
one

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