Propagate probe and variable dependencies into analog function output variables#85
Open
tvrusso wants to merge 3 commits intoQucs:developfrom
Open
Propagate probe and variable dependencies into analog function output variables#85tvrusso wants to merge 3 commits intoQucs:developfrom
tvrusso wants to merge 3 commits intoQucs:developfrom
Conversation
tvrusso
added a commit
to Xyce/Xyce
that referenced
this pull request
Aug 11, 2019
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
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
tvrusso
force-pushed
the
topic-issue67
branch
from
2c6a7db to
eb5011f
Compare
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
tvrusso
force-pushed
the
topic-issue67
branch
from
eb5011f to
0be3dbc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Per issue #67, the default implicit templates for ADMS (specifically e:dependency) do not correctly propagate probe and variable dependencies of input arguments into all output arguments. They only do this correctly for the function return value (by propagating these dependencies into the global expression containing the analog function call).
This PR augments e:dependency so that it treats analog function output variables in much the same way that it treats the left-hand side of assignments, propagating all probe and variable dependencies, and also setting flags such as "setinmodel", "setinevaluate", etc. just as assignments do.
This should fix issue #67 and should obsolete PR #68
I am also attaching a tarball that can be used to evaluate this PR. It contains:
rlc.va -- a simple series RLC model in Verilog A
rlc_AF.va --- the same model, but with all computation moved down into an analog function with multiple output variables
html_params.xml -- an ADMS "code generator" back-end that just makes an HTML file describing the module
xyceBasicTemplates.xml --- some templates that html_params.xml uses
two HTML files generated with html_params.xml from rlc.va and rlc_AF.va, using the fixed adms.implicit.xml file in this PR.
ADMS_AnalogFunctionIssue2.tar.gz
If one regenerates the HTML file from rlc_AF.va without using the fixes in this PR (e.g. with "admsXml -e xyceBasicTemplates -e html_params.xml rlc_AF.va"), one can see that the CapacitorCharge and InductorFlux variables are not properly marked with their dependencies on probes or parameters. As a result of this incorrect dependency tracking, the Jacobian is missing elements as well. Any real code generated from this model without these fixes would be wrong.