Skip to content

fix: function and arg size book-keeping for TestSub recursion#874

Open
jodavies wants to merge 2 commits into
form-dev:masterfrom
jodavies:issue-741-gpt
Open

fix: function and arg size book-keeping for TestSub recursion#874
jodavies wants to merge 2 commits into
form-dev:masterfrom
jodavies:issue-741-gpt

Conversation

@jodavies

Copy link
Copy Markdown
Collaborator

When TestSub is called recursively, correctly update the nested sizes. Rely on the recursion to fix the sizes at each level, except when an argument term disappears completely; then we fix up the whole nesting stack and continue processing further argument terms.

This fix was prepared with the help of an LLM report: https://gist.github.com/tueda/9d71a87e04dcc1f51fbbb432ae1af903

Its findings are in line with previous debugging efforts by me, but I did not figure out a proper fix at the time.

In addition:

  • completely remove the "subsubveto" mechanism, which appears to be a previous workaround, required due to the incorrect size updates
  • add a check that we do not exceed the FunctionLevels setup parameter, and give an error message for the user if so.
  • check RecFlag has the same value on entry and when returning from TestSub

This fixes #367 and #741

@jodavies

Copy link
Copy Markdown
Collaborator Author

Tentatively, this is also a 1% performance improvement in forcer, forcer-exp and maybe hyperform.

@coveralls

coveralls commented Jul 13, 2026

Copy link
Copy Markdown

Coverage Status

coverage: 63.183% (-0.003%) from 63.186% — jodavies:issue-741-gpt into form-dev:master

When TestSub is called recursively, correctly update the nested sizes.
Rely on the recursion to fix the sizes at each level, except when an
argument term disappears completely; then we fix up the whole nesting
stack and continue processing further argument terms.

This fix was prepared with the help of an LLM report:
https://gist.github.com/tueda/9d71a87e04dcc1f51fbbb432ae1af903

Its findings are in line with previous debugging efforts by me, but
I did not figure out a proper fix at the time.

In addition:
- add a check that we do not exceed the FunctionLevels setup parameter,
  and give an error message for the user if so.
- check RecFlag has the same value on entry and when returning from TestSub
@jodavies

jodavies commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator Author

I put back subsubveto, since @tueda is not sure about it. I think it would help if we could find a FORM script which would hit the following code, in the case that AN.subsubveto == 1:

form/sources/proces.c

Lines 741 to 748 in b143fc2

if ( t[3] == 0 ) {
t1 = r + r[1];
t2 = term + *term;
*term -= r[1]+t[1];
r = t;
while ( t1 < t2 ) *r++ = *t1++;
goto ReStart;
}

@jodavies

jodavies commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

While this fixes many examples, I think it the code is still not correct, but for slightly independent reasons. @cbmarini has been also looking at an issue with this same code:

CFunction rat;
Symbol ep;
Local F = rat(70944*ep^5,1);
Print "%r";
Multiply replace_(ep,100000);
Print "%r";
Print;
.end

Here the issue is the in-place call of Normalize here

Normalize(BHEAD t);

In this case, Normalize makes the term larger after raising 100000 to the fifth power: thus Normalize blindly over-writes the trailing term information.

We need to either, copy the term to the workspace and normalize there, and then copy back taking care of the size changes, or not Normalize at all at let the later Normalize during sorting deal with the terms. Then one has to instead make sure that terms which go to 0 are still handled correctly in the Nest stack book-keeping (which was why the Normalize was introduced in the first place).

If Normalize produces a larger result, trailing term data is overwritten.
@jodavies

Copy link
Copy Markdown
Collaborator Author

The second commit resolves the problem by not calling Normalize at all, during the recursion, but relying on the Normalization just before the following sort. The Nest stack is updated entirely by the recursive calls. I am not sure if it is guaranteed that TestSub never increases the size of the term, this does not happen in the test suite at least. The commit adds a Terminate in this case.

As far as I can tell, the code has now come "full circle" back to 2003, except that "subsubveto" is still there?

Any thoughts on this?

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.

replace_ fails to handle 0-replacement

2 participants