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

chore(comp): remove -v95 flag #719

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thoughtpolice
Copy link
Contributor

Remove all support code for Verilog 95 compatibility. Not only is V95 ancient, it also doesn't support necessary features like vendor-agnostic attributes, which we will want to use in the future.

This also deletes the relevant tests and simplifies a little of the pretty-printing code, too.

Attempt to reland the first part of #400 separately from everything else.

Remove all support code for Verilog 95 compatibility. Not only is V95 ancient,
it also doesn't support necessary features like vendor-agnostic attributes,
which we will want to use in the future.

This also deletes the relevant tests and simplifies a little of the pretty-
printing code, too.

Signed-off-by: Austin Seipp <aseipp@pobox.com>
@thoughtpolice thoughtpolice marked this pull request as draft July 18, 2024 23:35
@thoughtpolice thoughtpolice marked this pull request as ready for review July 19, 2024 13:59
@thoughtpolice
Copy link
Contributor Author

Note, I still want to think about addressing this original complaint from PR 400:

Ok, so, stepping back. The Maybe String on the left in vm_inst_params is to support situations where we know the names of the parameters, but have chosen to use unnamed instantiation -- the names are then printed as a comment next to the positional arguments. It seems like you've taken the position that, if you know the names, then you should use named parameters; therefore you've deleted the option of using positional with commented names. (I don't know if this was your explicit thinking or if you were led astray by the fact that the function for printing the commented names was called pv95params. That's a misnomer, since this syntax is allowed in later Verilog, so it could be better called pvPositionalParamNames, say.)

I'm OK with that decision, although I'd prefer to go a step further: remove the positional option altogether! (Remove the Either and just have the right side.) But, as I've shown above, there are still two places that use positional parameters, and removing that use might be outside the scope of this PR. In that case, I'd like to see two things: (a) comments on vm_inst_params explaining the structure and strongly suggesting not to use the Left side, and that it should be removed; and (b) PRs opened to improve noinline and to consider what to do about foreign.

The Maybe on the right in vm_inst_params is to support the generation of a submodule instantiation where a parameter name is mentioned, but no value is provided -- which I assume is equivalent to not mentioning parameter at all, but is slightly more self-documenting, by mentioning that we know the parameter exists and have chosen to let the module use its default value. BSC doesn't use this feature, so you've removed it. As I said above, if we think this is a generically useful feature, we could decide to leave it in the Verilog library; but I'm not sure how useful it is, so it's OK if you want to remove it.

And, I can bring back the Signed test, I'll do that. The other nits were addressed though, I think. I can sti

@quark17
Copy link
Collaborator

quark17 commented Sep 5, 2024

I'm OK with that decision, although I'd prefer to go a step further: remove the positional option altogether! (Remove the Either and just have the right side.)

I think this is OK to do. I said that there were some places still using it, but I can't find anything that triggers it. The only place that uses Left is in AVerilogUtil.hs:

vDefMpd vco (ADef i t
               (ANoInlineFunCall _ _
                  (ANoInlineFun n is (ips, ops) (Just inst_name)) es) _) _ =

and I have added the following code, to see if the situation ever arises:

  if (not (null is))
    then internalError ("ANoInlineFunCall with params")
    else

And that code is only ever triggered by expression constructed here in AConv:

aExpr e@(IAps (ICon i (ICForeign { fName = name, isC = False, foports = (Just ops)})) ts es) = do

where, again, I have added this code to test if there are every any parameters (that would need Left):

        when (not (null ns)) $
          internalError("ICForeign with params")

So far I haven't found any examples, but I'm running the full testsuite now.

@quark17
Copy link
Collaborator

quark17 commented Sep 5, 2024

The only failure in BSC's testsuite are in testsuite/bsc.lib/fork/ which is testing the Fork library that has this:

foreign vfork :: Bit n -> Bit m = "Fork",("i","o")

I also made a standalone example like this:

package TestForeignBH where

foreign f :: Bit a -> Bit a -> Bit a = "F",("i1" "i2","o")

{-# properties mkTest = { verilog } #-}
mkTest :: Module Empty
mkTest =
    module
      rgx :: Reg (Bit 1) <- mkRegU
      rgy :: Reg (Bit 1) <- mkRegU
      rgz :: Reg (Bit 1) <- mkRegU
      rules
           "r":  when True ==> action { rgz := f rgx rgy }

The parsing for this is the first line of pTyDefn in the Classic parser. It doesn't have any sanity checking -- if you leave off one of the argument names, you still get Verilog, but with a port missing. This syntax doesn't currently allow specifying the names of parameters, so it generates an instantiation with unnamed parameters -- but we could add that to the syntax. (The port names are optional; if left off, the imported function is treated as a Verilog function, not a module instantiaton.)

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