[libre-riscv-dev] [Bug 132] SIMD-like nmigen signal for partitioning

[bugzilla-daemon at libre-riscv.org]

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

--- Comment #32 from Jacob Lifshay <programmerjake at gmail.com> ---
(In reply to Luke Kenneth Casson Leighton from comment #31)
> (In reply to Jacob Lifshay from comment #30)
> 
> > I actually think it may be better to refactor the code to use Mux instead of
> > m.If and m.Else:
> 
> interesting. why? because, ultimately, yosys "proc" and "opt" end up
> replacing
> m.If/Else with Muxes anyway.
> 
> > 1. we won't have to modify nmigen's Module
> 
> i was thinking along the lines of a derived class (or a wrapper that takes
> Module as an argument).

I think wrapping or deriving from Module may make it unnecessarily extremely
complex.

> 
> > 2. it appears as though we have been subconsciously thinking that m.If
> > somehow makes the logic inside the m.If less expensive when the condition is
> > dynamically disabled. Having an explicit Mux may fix that.
> 
> i wasn't thinking :)
> 
> here's a simple example:
> 
> -        with m.If(self.i.product[-1]):
> -            comb += p.eq(self.i.product)
> -        with m.Else():
> -            # get 1 bit of extra accuracy if the mantissa top bit is zero
> -            comb += p.eq(self.i.product<<1)
> -            comb += self.o.z.e.eq(self.i.z.e-1)
> +        msb = Signal(reset_less=True)
> +        e = self.o.z.e
> +        comb += msb.eq(self.i.product[-1])
> +        comb += p.eq(Mux(msb, self.i.product, self.i.product<<1))
> +        comb += e.eq(Mux(msb, self.i.z.e, self.i.z.e-1))
> 
> that would do alright, wouldn't it? 

that looks fine to me.

> 
> my only concern is: where logically, the actions used to be "grouped"
> together (the "If" things are grouped together), now they're separated.

I think that most of the places where there's a large amount of code in a If
block (can't be replaced with a few Muxs), the code should have probably been
refactored anyway. the rest of the cases can be replaced with a simple mux
signal/variable like you did above.

> 
> > 3. we will need to review and, if needed, rewrite code anyway when
> > converting to partitioned form. rewriting to Mux may help us avoid missing
> > any code.
> 
> yes, as "grep Mux" would provide the identification-points.

grep Mux won't provide all the identification points since there is probably
some code that would need to be refactored that doesn't contain Mux or If.

> 
> for example, the msb above now clearly needs to be a "multi-partitioned"
> Signal.
> 
> > for part 3, we will need PartitionedValue to not be derived from nmigen's
> > Value to allow nmigen to catch mixups.

another reason to have PartitionedValue and Value separate is that we need
scalar (non-partitioned) as well as simd (partitioned) values in the same code.

> 
> hmmm hm hm hm... maaybe.
> 
> > It may also be a good idea to use a slightly different name for Mux since
> > they are different operations, how about MuxP for Mux-partitioned? that
> > would allow using a free function/class instead of a member.
> 
> yehyeh, that's a good idea.
> 
> sigh this is a big change, which can't be tested (live) without actually
> committing entirely to it.
> 
> can you think of a way to gain some confidence that this is going to work,
> *before* going ahead with the whole lot?

try it on divcore, which is relatively small but is complex enough to fully
utilise the operations we will need on partitioned signals.

the divcore tests can be adapted to work with partitioned signals relatively
simply.

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