-
Notifications
You must be signed in to change notification settings - Fork 31
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
Analog Function "output" and "inout" variables do not get marked with dependencies #67
Comments
thanks for the report -- it will be very useful to have more complete implicit rules... I had a first look, see #68. |
Looks like a good start. All of the dependencies (probe and variable) that would be transferred from RHS to LHS variables in an assignment should happen for output variables. Probe dependencies are of course needed for normal simulation, but variable (especially parameter) dependencies could be important for sensitivity analysis. |
I have been working on this issue and believe I may have a fix that completely propagates all dependencies from analog function inputs to all outputs in exactly the same way that it already does to the return value. I'm testing it out now, but so far it looks good. Should have a PR to submit by this evening. |
Per issue Qucs#67, the e:dependency template in adms.implicit.xml will propagate probe and variable dependencies from analog function arguments into the top-level expression in which the function is called (generally the RHS of an assignment), but does NOT do the same thing for any output arguments that might exist in the analog function. Some new CMC standard models (e.g. the MVSG-HV model) use analog functions in this manner, and it would be good to fix it. This commit augments the e:dependency template so that it completely propagates all dependencies from function arguments into any output variables that might exist. It is a no-op if there are no such function output arguments. This commit also obsoletes pull request Qucs#68. Unlike that PR, this one makes sure to perform dependency checking on the input arguments before copying their probe dependencies, and unlike that PR, it also copies their variable dependencies. Also unlike that code, it will correctly handle output arguments that are not at the beginning of the argument list, because it selects all arguments in the function definition in order and uses their position in the global list rather than selecting only the output arguments and using their position in the truncated list. Issue Qucs#67 PR Qucs#68
Analog Function output variables should definitely behave in every way as if they're just assignments. Assignments also set attributes like "setinmodel" or "setininstance", and so should these. Issue Qucs#67
This commit fixes an annoying and long standing issue with ADMS that impacts only models that have analog functions that return values in "output" arguments of their argument list. MVSG-HV is the only one that we have so far that has this issue. Fixing this issue was an impediment to finishing work on bug 1168. This work includes an update to the "implicit" templates used by ADMS. We now bypass use of the implicit templates used by ADMS and instead use our modified version. This was bug 1223. In addition to fixing the implicit dependency-tracking template, I had to update Xyce/ADMS's template that susses out what variables need to be declared. It had previously been not been written to treat use of output variables of analog functions as an assignment of sorts. Fixing these two issues removes the need for clumsy hacks in the MVSG-HV Verilog-A source. It has NO impact at all on any model that DOESN'T use analog functions in this way. The bug 1223 fixes have been fed back upstream to the Qucs/ADMS repo in pull request number 85 to address their issue number 67. https://joseki-vm.sandia.gov/bugzilla/show_bug.cgi?id=1168 https://joseki-vm.sandia.gov/bugzilla/show_bug.cgi?id=1223 Qucs/ADMS#67 Qucs/ADMS#85
Analog Function output variables should definitely behave in every way as if they're just assignments. Assignments also set attributes like "setinmodel" or "setininstance", and so should these. Issue Qucs#67
Per issue Qucs#67, the e:dependency template in adms.implicit.xml will propagate probe and variable dependencies from analog function arguments into the top-level expression in which the function is called (generally the RHS of an assignment), but does NOT do the same thing for any output arguments that might exist in the analog function. Some new CMC standard models (e.g. the MVSG-HV model) use analog functions in this manner, and it would be good to fix it. This commit augments the e:dependency template so that it completely propagates all dependencies from function arguments into any output variables that might exist. It is a no-op if there are no such function output arguments. This commit also obsoletes pull request Qucs#68. Unlike that PR, this one makes sure to perform dependency checking on the input arguments before copying their probe dependencies, and unlike that PR, it also copies their variable dependencies. Also unlike that code, it will correctly handle output arguments that are not at the beginning of the argument list, because it selects all arguments in the function definition in order and uses their position in the global list rather than selecting only the output arguments and using their position in the truncated list. Issue Qucs#67 PR Qucs#68
Analog Function output variables should definitely behave in every way as if they're just assignments. Assignments also set attributes like "setinmodel" or "setininstance", and so should these. Issue Qucs#67
In my other commits on this branch, I modified adms.implicit.xml to propagate probe and variable dependencies from analog function argument lists into output variables (other than the return value, which was already handled correctly). In doing so, I had neglected setting the "dependency" element of the ADMS data tree on those variables, and so they were incorrectly being set to "constant". This led to the incorrect setting downstream of the OPdependent element as well. This commit correctly sets "dependency", and therefore OPdependent (which is set later by probing "dependency"). Qucs#67
Just a quick bump to note that I have added one more commit to the pull request associated with this issue. The Xyce project is no longer using the adms.implicit.xml provided with ADMS and uses our own, and one of the things we have done is to fix this issue in our adms.implicitl.xml. I had submitted PR#85 when I first coded it up in the hopes of sharing that work with the larger ADMS user base, but afterwards extended the fix somewhat in Xyce without updating the PR. The PR now contains all of the analog-function-related fixes that Xyce uses and that wouldn't break existing back-ends. Without these fixes, Verilog-A models that use analog functions with multiple output variables will not have the dependencies propagated properly. Several CMC standard models (such as MVSG-HV (or as it is now called, MVSG-CMC)) make use of such analog functions, and code generators that make use of dependency information from the ADMS data tree will be getting incorrect information about these additional analog function outputs. |
In my other commits on this branch, I modified adms.implicit.xml to propagate probe and variable dependencies from analog function argument lists into output variables (other than the return value, which was already handled correctly). In doing so, I had neglected setting the "dependency" element of the ADMS data tree on those variables, and so they were incorrectly being set to "constant". This led to the incorrect setting downstream of the OPdependent element as well. This commit correctly sets "dependency", and therefore OPdependent (which is set later by probing "dependency"). Qucs#67
I discovered (or rather, re-discovered) this issue while trying to process the MVSG-HV HEMT model for use in Xyce. This model has a number of issues that complicated import into our simulator, but the hardest one to work around was this one.
ADMS correctly adds probe and other variable dependence to a variable's data in the data tree only if that variable appears on the left hand side of an assignment. This covers almost all of the uses in most common Verilog-A models in the wild.
However, the MVSG-HV model makes use of some complicated analog functions that not only return a value for the function, but also have "output" and "inout" variables in their argument lists. For all practical purposes, these amount to a potential assignment to the variables in those slots when the function is called.
ADMS will conservatively mark the variable on the left hand side of a call to such a function as depending on all the variables and probes in the argument list, but does NOT mark any of the output variables in this way. This can lead to many problems, the most significant of which could be that elements of the jacobian data structure might never be created, if the only connection between variables occurs through dependencies introduced inside an analog function. It also never marks "output" or "inout" variables as being assigned to, which complicates code generation (notably, if one is only declaring variables that get assigned to, as we do in Xyce's code generation templates).
I am attaching a tar file with some code that demonstrates this sort of problem. The tar file contains two Verilog-A models that should implement identical models. The first, "rlc.va" crudely implements a 2-terminal series RLC subcircuit in the most straightforward manner possible. The second, "rlc_AF.va" moves all of the computation of currents, charges, and fluxes into an analog function that is very much like some of those in MVSG-HV, but is much, much simpler.
Also included are two HTML representations of data extracted from the ADMS data tree by the "html_params.xml" ADMST template file (also included in the tar file). These HTML files can be viewed in any web browser. The HTML files are N_DEV_ADMSrlc.html (corresponding to rlc.va) and N_DEV_ADMSrlc_AF.html (corresponding to rlc_AF.va).
Looking at N_DEV_ADMSrlc_AF.html shows that ADMS has not marked the "CapacitorCharge" and "InductorFlux" variables as depending on anything at all. Correct variable dependencies are recorded for rlc.va, as shown by N_DEV_ADMSrlc.html. Note also that due to the dependency issue, the rlc_AF.va module is missing three of the jacobian elements found for rlc.va, and rlc_AF.va has two additional jacobian elements not found in rlc.va (the extra elements are due to the issue described two paragraphs down, and are not really an error).
For rlc_AF.va, ResistorCurrent is only getting any of the dependencies marked because it appears on the LHS of the assignment as well as being an output variable.
(Another difference here is that for rlc_AF.va, ADMS has marked ResistorCurrent as depending on all of the parameters and probes in the argument list, because it does this marking very conservatively, without a deep analysis of the analog function --- this is not a problem for us, but can produce an unnecessarliy dense jacobian stamp, as noted above).
I realize that fixing this may be a very difficult thing. But ideally, any variable that appears as an argument to an analog function for an "inout" or "output" variable should be treated exactly as if it had appeared on the LHS of an assignment for the purposes of determining variable and probe dependence.
As an aside, the "html_params.xml" template is one I created for exploring the ADMS data tree prior to importing new models into Xyce, and may be of interest to others (c.f. issue #66). To use it, run "admsXml -e xyceBasicTemplates -e html_params.xml" followed by the name of a Verilog-A input file. It will produce an HTML file with some useful tidbits about what appears in the parsed data tree.
ADMS_AnalogFunctionIssue.tar.gz
The only workaround I've been able to find to get MVSG-HV to work for us is to hack on the verilog source and construct additional assignment lines to trick ADMS into marking parameter and probe dependence, so that we can generate correct code from it. It is an ugly workaround.
The text was updated successfully, but these errors were encountered: