Skip to content

Conversation

jhawthorn
Copy link
Member

@jhawthorn jhawthorn commented Dec 12, 2022

We want to hint to the compiler that it's likely that the BOP is not likely to be redefined (the bit is 0). Previously we were accidentally hinting to the compiler that it was non-zero (it was redefined) due to a misplaced parenthesis. Oops 😅.

This likely (though I'm still not 100% sure) was the cause of an MJIT regression from #6851 on nbody, optcarrot, and similarly math-heavy benchmarks (and it can't have helped the interpreter, though I haven't been able to see evidence of that). It also might depend on the compiler, I was never able to reproduce this on gcc 12.2 on arch linux, but was able to see a difference inside an ubuntu docker container built with gcc 9.4.

I was able to test this most easily by using:

time ruby --mjit --mjit-verbose --disable-gems -e 'a=0; 100_000_000.times { a = a+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1 }'

cc @k0kubun @composerinteralia

We want to hint to the compiler that it's likely that the BOP is
unredefined (the bit is 0). Previously we were accidentally hinting to
the compiler that it was non-zero due to a misplaced parenthesis.
@composerinteralia
Copy link
Contributor

Oof, good catch 🤦🏻

likely (though I'm still not 100% sure)

Should that be:

LIKELY(though I'm still not 100% sure)

😜

@k0kubun
Copy link
Member

k0kubun commented Dec 12, 2022

I confirmed that this fixes the problem. Thank you so much!

before: ruby 3.2.0dev (2022-12-12T21:05:03Z master f88f2bd92f) +MJIT [x86_64-linux]
after: ruby 3.2.0dev (2022-12-12T21:24:04Z bop-likely-parens 1a50f65c4a) +MJIT [x86_64-linux]

-----  -----------  ----------  ----------  ----------  ------------  -------------
bench  before (ms)  stddev (%)  after (ms)  stddev (%)  before/after  after 1st itr
nbody  49.4         0.1         35.1        0.2         1.41          1.14
-----  -----------  ----------  ----------  ----------  ------------  -------------

@jhawthorn jhawthorn merged commit 43f9351 into ruby:master Dec 12, 2022
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.

3 participants