Suggestion for clock and reset recognition

[fukatani]

Currently, pyverilog recognize clock and reset signal by their name at bind_visitor._createAlwaysinfo.

But I hope to be implemented other method using if branch structure (as command-line option or default).

That is, if the first "if" branch conditions is a signal in the sensitivity list, it should be determined as reset.

i.e.

def _is_reset(self, node, sig):
    if not isinstance(node.statement.statements[0], pyverilog.vparser.ast.IfStatement):
        return False
    if isinstance(node.statement.statements[0].cond, pyverilog.vparser.ast.Ulnot) and edge == 'negedge':
        target = node.statement.statements[0].cond.children()[0]
    elif edge == 'posedge':
        target = node.statement.statements[0].cond
    else:
        return False

    if isinstance(target, pyverilog.vparser.ast.Pointer):
        if sig.ptr == target.ptr:
            target = target.var
        else:
            return False

    return target == sig

I look forward to listening your opinion when there is your time.

[shtaxxx]

Thank you for a good suggestion.

As you said, the current isReset and isClock are defined just by using a simple rules of signal names, and it is possible to detect reset signals by using sensitivity list.

However, especially FPGA development, synchronized reset is recommended rather than asynchronous reset. Just by using sensitivity lists, it is impossible to find out reset signals.

Do you or anyone have a good idea?

[fukatani]

Thank you for your reply.

Current bindvisitor.alwaysinfo doesn't include synchronized reset information. (Because current createAlwaysinfo search signals only in node.sens_list.list).
And I hope to implement special method for bindvisitor._createAlwaysinfo, not for another place.
So I think that not corresponding to synchronized reset is no problem.

If we have to detect syncronized reset, I think first if branch is judged material.
If it substitute wire or reg variable (not constant), syncronized reset is not exists.

ex.

always @(posedge clk) begin
        if (rs)
            q_reg <= 1'b0; // rs is syncronyzed reset
        else
            q_reg <= d;
    end 

always @(posedge clk) begin
        if (rs)
            q_reg <= d; // q_reg don't load constant at first if branch, so rs is not syncronyzed reset
        else
            q_reg <= d;
    end 

[shtaxxx]

Thank you for a nice idea. I think it can be implemented within a small effort. After that, let's replace the current name-based implementation with it.

[fukatani]

Thanks! I agree with your roadmap. I'll try implementing it and do PR.

[fukatani]

I found subtle issue about reset detection for testcode/vectoradd.v.

  always @(posedge CLK or negedge RST_X) begin
    if(!RST_X) begin
      cnt0 <= 0;// RST_X is not reset for MEM_WE
    end else begin
      if(state == ST_INIT) begin
        MEM_WE <= 0;
    end
  end

This case, in current Pyverilog, RST_X is regarded as reset signal of MEM_WE.
But in my PR version, RST_X is not regarded as reset, so raise Exception in _createAlwaysinfo.

1. Completely same as current Pyverilog.
2. Raise Exception as my closed PR.
3. Not regard as reset, but not raise Exception.

Which is best choice?

[shtaxxx]

I'm sorry for the delay.
Actually I did not understand the behavior of the updated version of bind_visitor._createAlwaysinfo().
I think choice 3 is the best currently.

[fukatani]

いえ、私のほうも忙しかったり先にtravis入れようとして結構てこずったりしているので…。お時間あるときの返信で大丈夫です。
ええっと一応ちゃんと説明すると私が実装しようとするバージョンでは最初のif文の条件式にない信号をリセットとして認識しません。同期非同期問わず。
なので上記のようなコードはRST_XはMEM_WEにとってリセットではないと認識され、senslistに追加されます。
ところが、これはbindvisitor._createalwaysinfoに前からあったコードですが、

        if clock_edge is not None and len(senslist) > 0:
            raise verror.FormatError('Illegal sensitivity list')

という記述があり、ここで例外になってしまいます。そこで、一つのalwaysブロック内に複数の信号への代入が存在する場合、どこかでリセットとして使われていれば例外としない、というような処理に変えようと思ってます。

もう一つ、現状framesのalwaysinfoは各alwaysブロックごとに一つのCLK/RST情報を保持していますが、RST_XをMEM_WEのリセットにさせないために、一つのalwaysのなかで複数のCLK/RST情報を持たせる変更をします。

travis終わったらこの方針で実装しますが、イメージと違うなあと思ったら止めてください。