YJIT: upstream yjit_backend_ir branch (new backend)
#431
Comments
maximecb
changed the title
yjit_backend_ir branch (new backend)
|
Sounds good to me 👍 :) |
|
The noah_backend_rebase branch has now 'replayed' up through commit b9199af. That's about where we start seeing merge commits and other things of interest that need to be handled more carefully. It's also the earlier history of the new backend, where a lot of the commits don't seem to be part of any PR -- they were just pushed to the branch. As yet I haven't squashed basically anything that isn't part of a PR, so presumably it'll all be compressed a bit later. I've also made sure the PR commits have their PR number in the first line, so we can easily assess which are in-PR versus non-PR commits with "git log --online". And now, time to replay more history. I'll continue to push to noah_backend_rebase as I go along. |
|
I just updated yjit.md on the |
|
Did anything changed about Code GC? |
We will resume work on code GC in September. |
|
Let us know if you need help with anything Noah. |
|
So far, so good. Similarly, not too worried about some of it getting backported to master. It'll be fine. Still untangling right now. You can see progress on noah_backend_rebase. Once that's done (Friday?), the main thing I'll want help with is a decision about how much commit-squashing is to be done. That should be pretty straightforward, though. Somebody else could also look through test_yjit_new_backend.rb for usable bits and add them to another test file, come to think of it. I haven't started that yet and it doesn't depend on the rest of this task in any way. |
|
Good progress today. I've rolled history forward to yjit_backend_ir latest, making sure things build and don't look too bad along the way (but not running most tests.) Commits are linearised and squashed, messages include URLs for PRs, etc.
After that, I'll start diffing our code to make sure I didn't include or exclude anything obviously incorrect, run tests, and fix up any problems I find while doing all that. Things other people could now help with:
But I think we're getting close. |
|
Good work Noah! :) @k0kubun do you have bandwidth to help Noah with the upstreaming work? If so, you two could coordinate. You could pair and Noah could show you where he is at.
I was thinking we should take inventory of what's
What do you mean by remove our changes to the test code? |
For instance, we disable and re-enable the Ubuntu tests. We started by turning off a lot of standard tests, then slowly re-enabled them. In a lot of places, that'll cause churn in the files for those tests (e.g. stuff in .github/workflows, .cirrus.yml). |
Ok I see. Yes I think it would be good to remove those changes if we can to get a "cleaner" PR with less churn, but this can be lower on the priority list if you want since it probably wouldn't stop the PR from being merged. |
|
Great work! It looks pretty close to the merge.
Yes, I'd be happy to. It'd be a lot of force-pushes, so coordination is certainly needed. I'll talk with Noah.
I'll help him on those items once he signs off for today. |
Looking at the commit messages, the history already doesn't look too bad. I'm thinking about doing the following.
|
I noticed that you did it by appending For squash-merged ones with |
|
Here's the code I used for the link rewrite: #!/usr/bin/env ruby
message = STDIN.read
if message.lines.first.match?(/\(#\d+\)\n\z/)
message.sub!(/\(#(\d+)\)/, '(https://github.com/Shopify/ruby/pull/\1)')
message.sub!(%r[\nPR: https://github\.com/Shopify/ruby/pull/\d+], '')
message.chomp!
puts message
else
print message
end |
|
To fix test code, it'd be useful to have a buildable YJIT in every revision, so I started working on it first. After the filter-branch, I added the following modifications to my branch (at 5fd3754a42 now), and now every revision is buildable on x86_64 🎉
I wrote the following script commits = `git --no-pager log --date=iso --no-show-signature --pretty=format:'%h %Cgreen%ad %Cblue%an %Creset%s %C(blue)%d%Creset' 881bc2a176..k0kubun_backend_rebase`.lines.reverse
commits.each_with_index do |commit, index|
puts "=" * 100
puts "[#{index+1}/#{commits.size}] #{commit}"
puts "=" * 100
system("git checkout #{commit.split(" ").first}", exception: true)
system("make -C .yjit-x86_64 -j8", exception: true)
endand it ran successfully. |
|
So I'm looking at this range 881bc2a...baf0d8b to look for temporary CI/test changes now. We hadn't had the temporary workflow in the first 78 commits. Add temporary workflow to check new backend was the first commit that started doing the temporary testing. It'd be difficult to manually list up commits touching temporary CI/tests from 159 commits, so I listed up such commits with a script: lines = `git --no-pager log --date=iso --no-show-signature --pretty=format:'%h %Cgreen%ad %Cblue%an %Creset%s %C(blue)%d%Creset' 881bc2a176..k0kubun_backend_rebase`.lines.reverse
lines.each do |line|
revision = line.split(" ", 2).first
changed_files = `git diff #{revision}^ #{revision} --name-only`.lines.map(&:chomp)
if changed_files.any? { |f| f == '.cirrus.yml' || f.start_with?('.github/') || f.start_with?('bootstraptest/') }
puts line
end
end79 commits have been found. I'm gonna look into those commits. Bare output of the scriptList of commits with links
|
|
As to the CI script, I'd say the current |
Commits with changes under .github/
I finished removing changes under |
|
I'll call it a day here. The only remaining effort would be to remove duplication between FYI, here's the list of 44 commits that touch Commits touching bootstraptest/
|
|
Looks pretty good Kokubun. Thanks for all the work you've put into this. I just pushed 467f89d to remove |
|
This looks like great work. Thank you!
Yeah. I think we should remove it from the first line and just have the URL at the bottom. You're right, it gives bad links for ruby/ruby. It was useful for tracking the rebase, though! |
|
I've started on the commits for test_yjit_new_backend. |
|
(Sorry for the comments turning into headings in the details text) Stuff from test_yjit_new_backend that potentially seem useful/different to me:Line 352-372
# invokebuiltin
assert_equal '123', %q{
def foo(obj)
obj.foo = 123
end
struct = Struct.new(:foo)
obj = struct.new
foo(obj)
}
|
|
I wrote:
Ah, but you had done it the other way. Okay, I'm officially okay with either one ;-) |
|
I've rebased from Kokubun's branch to move the tests we want to keep to test_yjit.rb, and pushed it as branch noah_backend_rebase again. |
|
Nothing new in yjit_backend_ir. Sounds like we're basically okay on cirrus.yml. Looks like Kokubun already cleaned up the .github/workflows files? So we may be good to open the ruby/ruby PR. |
|
The branch looks great to me. Thank you for working on the test_yjit_new_backend portion. I'll do some final checks and then open a ruby/ruby PR. |
|
Perfect. Thanks! |
|
I finished checking what I wanted to check https://shopify.slack.com/archives/C019MPFLWHY/p1661536712906259. Next, I'll merge Alan's PR as the final patch, merge it to your branch, fix its PR link, rebase it from the latest master, run yjit-bench, and finally, file a ruby/ruby PR. |
|
At this point, any further work will take place in the ruby/ruby PR. Closing. We do still need to cherry-pick one more PR into the ruby/ruby PR (#437). But we'll do that there, not here. |
Starting from yjit_backend_ir (currently: d17b53c), I'll create a new branch without merge commits and with some level of commit-squashing. TODO items include:
Shopify/rubyPR URL is in the commit message (git filter-branch?)yjit.mdcirrus.yml; grab anything useful fromtest_yjit_new_backend.rb. Remove temp backend tests workflow.yjit_backend_irI'm biasing toward fewer commit-squashes early in the process, since it's easier to squash than un-squash if needed later.
The text was updated successfully, but these errors were encountered: