Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] get rid of that += and .eq() #608

Closed
github-4o opened this issue Apr 11, 2021 · 4 comments
Closed

[RFC] get rid of that += and .eq() #608

github-4o opened this issue Apr 11, 2021 · 4 comments
Labels

Comments

@github-4o
Copy link

github-4o commented Apr 11, 2021

replace m.d.<domain> += <target>.eq(<val>) with m.d.<domain>.<target> = <val>

implementation as runtime fix:

from nmigen.hdl.dsl import _ModuleBuilderDomain as b


def fix_semantix():
    def mysetattr(self, name, val):
        if name == "_domain":
            self.__dict__[name] = val
        else:
            if name in self._builder.__dict__:
                self.__iadd__(self._builder.__dict__[name].eq(val))
            else:
                raise Exception("couldn't find {} signal on the module".format(name))
    setattr(b, "__setattr__", mysetattr)

why?

  • more eye-pleasing cause you are used to it in other languages

pros: none, just a semantics fix

cons:

  • __setattr__ looks like a dirty hack that can create problems in the future
  • requires m.<target> = Signal(...) prior to m.d.<domain>.<target> = <value>
@whitequark whitequark added the rfc label Apr 11, 2021
@whitequark
Copy link
Member

whitequark commented Apr 11, 2021

The left-hand side of .eq() can be more complex than just a signal name. For example, you can do:

m.d.sync += buf.word_select(idx, 4).eq(input)

This is not representable with the syntax you're suggesting.

(Aside from that, this proposal violates Python name scoping in a way far more substantial than what nMigen already does, but that's a secondary issue compared to the one above.)

@ildus
Copy link

ildus commented Jul 12, 2021

@whitequark how about this approach: m-labs/nmigen#345? I created the issue in the wrong repository by mistake.

@modwizcode
Copy link
Contributor

modwizcode commented Dec 11, 2021

Personally, we find that this sort of change obscures important details about what's actually occurring when you assign a signal in a domain like this. You're adding a new assignment statement and the code itself should reflect that. Beyond the potential implementation issues, this fact becomes important because later assignments can override earlier assignments that have been added.

@whitequark
Copy link
Member

whitequark commented Apr 5, 2022

The proposal as submitted has a fatal issue: it makes a substantial amount of existing code unrepresentable.

The proposal as modified later has a fatal issue: the code style it proposes makes it impossible to reuse the decision trees built through If/Switch statements to drive both combinatorial and synchronous signals in the same statement body (or at least the same decision tree). This was one of the substantial issues with Migen that Amaranth aimed to address.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

4 participants