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

Add more exprjit templates (07/08/18) #900

Closed
wants to merge 25 commits into from

Conversation

jstuder-gh
Copy link
Contributor

@jstuder-gh jstuder-gh commented Jul 8, 2018

Add 59 more templates and nullptr macro.

They've passed the spectest with the following env vars set:

  • MVM_SPESH_NODELAY=1
  • MVM_SPESH_INLINE_DISABLE=1
  • MVM_SPESH_OSR_DISABLE=1

@lizmat
Copy link
Contributor

lizmat commented Jul 12, 2018

Thank you for all the hard work. But I think these should be added as separate Pull Requests, so they can be judged separately, and bisected better in case of problems.

Now, it is an all-or-nothing Pull Request, which gets scarier to commit with each addition.

@jstuder-gh
Copy link
Contributor Author

I understand and appreciate your concern. I think that you're right that it would be better to submit in smaller, digestable chunks than with one monolithic PR. In the future, I'll definitely submit more along those lines.

I do want to mention that I have been working on this branch with bisectability in mind. Each commit only has a few related templates included at a time (if they are only calling a C function) or only one if it is a bit more involved.

I run this script as I add new commits and use it with git bisect to track down issues when they appear. This script runs the spectest and sets MVM_SPESH_NODELAY so that the jitting will occur right away (as well as MVM_SPESH_INLINE_DISABLE and MVM_SPESH_OSR_DISABLE so that there is less optimization going on other than the jitting).

This branch passes for me when I run this with commit 2ff4274d6482eafadce42c06b9763b6d084ab277 checked out for the roast (it's a bit older, but so is the head of MoarVM's master that I branched off from). I'm also using commit cec76ff79ebb4092803d5e680e648883bda1bc55 on NQP and commit ade83c861484f5f4e4184a88d9dae79a2cdf110c on Rakudo.

This script is assuming that all the source directories have the same base directory and that MoarVM has already had Configure.pl run for it. Also I tend to run it with a local branch other than master checked out on ${basedir}/rakudo/t/spec so that the spectest won't be updated (there's probably a better way but it works).

I won't add any more commits to this branch. If need be I can break this one up too.

@lizmat
Copy link
Contributor

lizmat commented Jul 15, 2018

Wrt this branch, I'll leave that up to the people deciding the correctness of it. :-)

@jstuder-gh
Copy link
Contributor Author

Closing (was merged in commit 691aa0d).

@jstuder-gh jstuder-gh closed this Jul 21, 2018
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.

None yet

2 participants