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

Tail recursion elimination for GenC (#1275) #1626

Merged
merged 32 commits into from
Jan 10, 2025

Conversation

zhekai-jiang
Copy link
Contributor

@zhekai-jiang zhekai-jiang commented Jan 5, 2025

This is to address #1275 . We added a new phase to perform tail recursion elimination.

We added test cases in the GenCSuite.

As discussed during the presentation, we may want to take a closer look at ghost elimination and see whether it is doing the job correctly.

@CLAassistant
Copy link

CLAassistant commented Jan 5, 2025

CLA assistant check
All committers have signed the CLA.

@zhekai-jiang zhekai-jiang changed the title Tail recursion elimination for genc (#1275) Tail recursion elimination for GenC (#1275) Jan 5, 2025
@zhekai-jiang zhekai-jiang marked this pull request as ready for review January 5, 2025 16:29
@vkuncak
Copy link
Collaborator

vkuncak commented Jan 6, 2025

Verification of TailRecComplexArgs.scala failed on the CI.

@vkuncak
Copy link
Collaborator

vkuncak commented Jan 6, 2025

@zhekai-jiang
Copy link
Contributor Author

Could we try to run the tests again? (Apparently it was because we didn't add the decreases measures for our new test cases, so Stainless was not able to verify everything, but this issue was not related to the generated C code.)

@zhekai-jiang
Copy link
Contributor Author

By the way, I removed the test case TailRecReturnInLoop because apparently this kind of thing is not allowed by Stainless, because the loop will be rewritten into a function, and if we make a recursive call in the loop, it will make the inner function mutually recursive with the enclosing function and Stainless throws an error (which I think is from

val errorInfo = tc.currentFid flatMap { currentFid =>
val currentDeps = dependencies(currentFid)
val mutuallyRecursiveDeps = currentDeps.filter { did =>
dependencies(did).contains(currentFid)
}
if (mutuallyRecursiveDeps.contains(id)) {
Some(s", because it is mutually recursive with the current function ${currentFid.asString}")
} else {
None
}
}
reporter.fatalError(in.getPos,
s"Call to function ${id.asString} is not allowed here${errorInfo.getOrElse("")}"
)
)

@samuelchassot
Copy link
Collaborator

I rerun the CI 👍

By the way, I removed the test case TailRecReturnInLoop because apparently this kind of thing is not allowed by Stainless, because the loop will be rewritten into a function, and if we make a recursive call in the loop, it will make the inner function mutually recursive with the enclosing function and Stainless throws an error (which I think is from

val errorInfo = tc.currentFid flatMap { currentFid =>
val currentDeps = dependencies(currentFid)
val mutuallyRecursiveDeps = currentDeps.filter { did =>
dependencies(did).contains(currentFid)
}
if (mutuallyRecursiveDeps.contains(id)) {
Some(s", because it is mutually recursive with the current function ${currentFid.asString}")
} else {
None
}
}
reporter.fatalError(in.getPos,
s"Call to function ${id.asString} is not allowed here${errorInfo.getOrElse("")}"
)

)

About that: can't we keep the benchmark with the while loop then?

@samuelchassot
Copy link
Collaborator

Oh sorry, they're all new test cases! My bad!

@zhekai-jiang
Copy link
Contributor Author

I see 3 failed tests, but they are not related to our part. Anyone has any idea why...?

@samuelchassot
Copy link
Collaborator

@vkuncak They're the same failing tests as on the other PR.
That's even more puzzling

@samuelchassot
Copy link
Collaborator

@sankalpgambhir could it be related to some Princess versions? They're not changing anything in the build file though.

@samuelchassot
Copy link
Collaborator

We investigated and these tests failing have nothing to do with your PR. It is happening because of the latest release of the Princess Solver. We will ignore those tests in a soon-to-be-merged PR.
You'll be able to update your branch and the CI should then pass :)

@vkuncak vkuncak merged commit 233cdcd into epfl-lara:main Jan 10, 2025
3 checks passed
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.

5 participants