Skip to content

Conversation

@ethanuppal
Copy link

Description & Motivation

TODO

Related Issue(s)

Testing

Backwards-compatibility

Is this a breaking change that will not be backwards-compatible? If yes, how so?

Documentation

Does the change require any updates to documentation? If so, where? Are they included?

name: name,
);

ComplexFloatingPoint adder(ComplexFloatingPoint other) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not override operator + and *?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to hide the fact that it's expensive

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the names of the functions seem unintuitive to me, maybe plus and times or addedTo and multipliedBy or something?

if we wanted to stay more consistent with other parts of the library, these could be their own components (classes, modules) e.g. ComplexFloatingPointMultiplier, perhaps with an argument that allows you to choose your own FloatingPointMultiplier implementation for the internals.

Signed-off-by: Ethan Uppal <[email protected]>
required this.inB,
required this.twiddleFactor,
super.name = 'butterfly'}) {
addInput('inA', inA, width: inA.width);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heads up, you'll want to use the signal returned by addInput or input for internal computation rather than the one provided in the constructor

Signed-off-by: Ethan Uppal <[email protected]>
name: name,
);

ComplexFloatingPoint adder(ComplexFloatingPoint other) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the names of the functions seem unintuitive to me, maybe plus and times or addedTo and multipliedBy or something?

if we wanted to stay more consistent with other parts of the library, these could be their own components (classes, modules) e.g. ComplexFloatingPointMultiplier, perhaps with an argument that allows you to choose your own FloatingPointMultiplier implementation for the internals.

import 'package:rohd_hcl/src/arithmetic/floating_point/fft/butterfly.dart';
import 'package:rohd_hcl/src/arithmetic/signals/floating_point_logics/complex_floating_point_logic.dart';

class BadFFTStage extends Module {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why "Bad"? :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not fully pipelined

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe "multi-cycle" is a better name or something (if that's what it is)?

required this.twiddleFactorROM,
super.name = 'badfftstage'})
: assert(ready.width == 1) {
addInput('clk', clk);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reminder: you'll want to do something like clk = addInput('clk', clk); instead of just taking this.clk as the argument, since the signals received as arguments to the constructor are not the input ports of this module.

@@ -0,0 +1,35 @@
// Copyright (C) 2025 Intel Corporation
// SPDX-License-Identifier: BSD-3-Clause
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please update headers on the files to match the recommended style, including the name of the file, date authored, purpose of the file, etc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably worth naming this something better than bad?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete this file?


late final Logic done;

BadFFTStage({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please do write doc comments

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remember to export components that are intended to be part of the public API for ROHD-HCL

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add user guide descriptions on how to use new components in the doc/ directory (including links to the pages)

return complex;
}

class Complex {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this class is defined in multiple files?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a place for shared utilities in tests?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can create a directory in either lib (not exported) or in the test directory and can reference other files that way

@ethanuppal ethanuppal marked this pull request as ready for review August 14, 2025 22:47
Signed-off-by: Ethan Uppal <[email protected]>
Signed-off-by: Ethan Uppal <[email protected]>
Signed-off-by: Ethan Uppal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants