search

xlsynth / bedrock-rtl

High quality and composable base RTL libraries in SystemVerilog
Apache License 2.0
11 stars 2 forks source link

Add output valid signal to onehot2bin module. #133

Closed mattskl-openai closed 2 weeks ago

mattskl-openai commented 2 weeks ago

Drive by comment. The implementation looks more like a priority encoder than one-hot to bin. Should we change this:

  if (in[i]) begin
    out = BinWidth'(i);
    break;
  end

To this?

 if (in[i]) begin
    out |= BinWidth'(i);
  end

Happy to make the change; is it typical to make multiple changes in one PR?

btowles-openai commented 2 weeks ago

Drive by comment. The implementation looks more like a priority encoder than one-hot to bin. Should we change this:

  if (in[i]) begin
    out = BinWidth'(i);
    break;
  end

To this?

 if (in[i]) begin
    out |= BinWidth'(i);
  end

Happy to make the change; is it typical to make multiple changes in one PR?

If you think the change is correct, then up to you whether it should be a separate PR. Yeah, usually best to keep separate logical changes in separate PRs ... but to a point ... if the changes are all to the same file and simple, for example, I think it's okay to save the overhead of two PRs. This is all subjective, of course.

mgottscho commented 2 weeks ago

Drive by comment. The implementation looks more like a priority encoder than one-hot to bin. Should we change this:

  if (in[i]) begin
    out = BinWidth'(i);
    break;
  end

To this?

 if (in[i]) begin
    out |= BinWidth'(i);
  end

Happy to make the change; is it typical to make multiple changes in one PR?

If you think the change is correct, then up to you whether it should be a separate PR. Yeah, usually best to keep separate logical changes in separate PRs ... but to a point ... if the changes are all to the same file and simple, for example, I think it's okay to save the overhead of two PRs. This is all subjective, of course.

Thanks for the catch, @btowles-openai! I'm fine with merging this fix in this PR