Elif without preceding If causes Syntax error when Elif has a complex condition

[alanvgreen]

I'm getting a syntax error when using a function call that creates a signal inside an Elif.

I can understand why this kind of function call might be problematic in a Python DSL, however I'm raising a bug because this code looks like it ought to be valid. If it's breaking a rule, I'm not sure what the rule is.

from amaranth import *
from amaranth.back import verilog

class Stepper(Elaboratable):
  def __init__(self):
    self.output = Signal(3)
    self.up = Signal()
    self.down = Signal()
    self.ports = [self.output, self.up, self.output]

  def edge_detect(self, m, input):
    last = Signal()
    m.d.sync += last.eq(input)
    detected = input & ~last
    return detected

  def elaborate(self, platform):
    m = Module()
    with m.If(self.edge_detect(m, self.up)):
      m.d.sync += self.output.eq(self.output+1)
    with m.Elif(self.edge_detect(m, self.down)):
      m.d.sync += self.output.eq(self.output-1)
    return m

s = Stepper()
verilog.convert(s, name='Top', ports=s.ports)

fails with:

[/usr/local/lib/python3.7/dist-packages/amaranth/back/verilog.py](https://localhost:8080/#) in convert(elaboratable, name, platform, ports, emit_src, strip_internal_attrs, **kwargs)
     48         warnings.warn("Implicit port determination is deprecated, specify ports explictly",
     49                       DeprecationWarning, stacklevel=2)
---> 50     fragment = ir.Fragment.get(elaboratable, platform).prepare(ports=ports, **kwargs)
     51     verilog_text, name_map = convert_fragment(fragment, name, emit_src=emit_src)
     52     return verilog_text

[/usr/local/lib/python3.7/dist-packages/amaranth/hdl/ir.py](https://localhost:8080/#) in get(obj, platform)
     35                 code = obj.elaborate.__code__
     36                 obj._MustUse__used = True
---> 37                 new_obj = obj.elaborate(platform)
     38             elif hasattr(obj, "elaborate"):
     39                 warnings.warn(

[<ipython-input-20-74600c1d41a4>](https://localhost:8080/#) in elaborate(self, platform)
     19     with m.If(self.edge_detect(m, self.up)):
     20       m.d.sync += self.output.eq(self.output+1)
---> 21     with m.Elif(self.edge_detect(m, self.down)):
     22       m.d.sync += self.output.eq(self.output-1)
     23     return m

[/usr/lib/python3.7/contextlib.py](https://localhost:8080/#) in __enter__(self)
    110         del self.argElif s, self.kwds, self.func
    111         try:
--> 112             return next(self.gen)
    113         except StopIteration:
    114             raise RuntimeError("generator didn't yield") from None

[/usr/local/lib/python3.7/dist-packages/amaranth/hdl/dsl.py](https://localhost:8080/#) in Elif(self, cond)
    251         if_data = self._get_ctrl("If")
    252         if if_data is None or if_data["depth"] != self.domain._depth:
--> 253             raise SyntaxError("Elif without preceding If")
    254         try:
    255             _outer_case, self._statements = self._statements, []

SyntaxError: Elif without preceding If

However, when the condition is calculated outside the Elif, it works

from amaranth import *
from amaranth.back import verilog

class Stepper(Elaboratable):
  def __init__(self):
    self.output = Signal(3)
    self.up = Signal()
    self.down = Signal()
    self.ports = [self.output, self.up, self.output]

  def edge_detect(self, m, input):
    last = Signal()
    m.d.sync += last.eq(input)
    detected = input & ~last
    return detected

  def elaborate(self, platform):
    m = Module()
    is_down = self.edge_detect(m, self.down)
    with m.If(self.edge_detect(m, self.up)):
      m.d.sync += self.output.eq(self.output+1)
    with m.Elif(is_down):
      m.d.sync += self.output.eq(self.output-1)
    return m

s = Stepper()
verilog.convert(s, name='Top', ports=s.ports)

[whitequark]

with m.If(self.edge_detect(m, self.up)):

The use of m both in a calling function and in a callee in this way is not supported. This is true even if you are not using self.edge_detect(m, ...) inside of a conditional expression (see below), but when used in a conditional expression it just breaks.

The reason it's not supported is that the following code (still using your edge_detect function):

    is_down = self.edge_detect(m, self.down)
    with m.If(...):
      m.d.sync += self.output.eq(is_down)

has very different semantics from the code below:

    with m.If(...):
      is_down = self.edge_detect(m, self.down)
      m.d.sync += self.output.eq(is_down)

which does not match how Python functions, variables, and scoping normally work, and is very likely to be a source of obscure bugs, as the edge detector circuit will gain an additional enable input that will no longer make it work like an edge detector.

[whitequark]

That said, the error it's currently raising is nonsensical, and the rule I'm talking about should be a warning (or an error), rather than something that is silently accepted.

I also have some ideas on letting people use functions without the scoping hazard I described, though that would need an RFC.

[alanvgreen]

Thanks for taking the time to write such a complete response. I learned something new.

The use of m both in a calling function and in a callee in this way is not supported.

I think I see the problem I created: by calling edge_detect() inside the Elif condition, perhaps the inverse condition of the If applies to it. If that is so, then it the code snippet created by edge_detect() has an extra enable input, and last created by edge_detect() is only updated when the If condition is false. In short, that code is ambiguous and hard to understand, and I ought to write it differently.

I tend to lean into using Python as a macro language for Amaranth, which is convenient, but m gets passed around a lot. Some guidance (as an error message or extra documentation) around when functions that use m can be called and when not would be very helpful.

[whitequark]

Some guidance (as an error message or extra documentation) around when functions that use m can be called and when not would be very helpful.

I agree. Let's keep ths open, since at the very least the error you're hitting is wrong.