search

facebookincubator / cinder

Cinder is Meta's internal performance-oriented production version of CPython.
https://trycinder.com
Other
3.42k stars 122 forks source link

Simplify unbox(box(x)) to x #106

Closed tekknolagi closed 1 year ago

tekknolagi commented 1 year ago

This happens in Static Python C-style loops.

Screenshot from 2023-04-17 11-45-00

tekknolagi commented 1 year ago

Looks like tests pass: https://github.com/tekknolagi/cinder/actions/runs/4722627505

tekknolagi commented 1 year ago

I should probably add more specific Simplify tests...

jbower-fb commented 1 year ago

Thanks for working on this. Should I wait for you to add a commit with the further simplify tests?

tekknolagi commented 1 year ago

Yeah, adding another shortly

On 4/19/23, Jacob Bower @.***> wrote:

Thanks for working on this. Should I wait for you to add a commit with the further simplify tests?

-- Reply to this email directly or view it on GitHub: https://github.com/facebookincubator/cinder/pull/106#issuecomment-1514114761 You are receiving this because you authored the thread.

Message ID: @.***>

tekknolagi commented 1 year ago

Done! Take it away as you see fit, @jbower-fb

facebook-github-bot commented 1 year ago

@jbower-fb has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

swtaarrs commented 1 year ago

You need to verify that the types of the two operations are compatible in a way that guarantees there won't be an OverflowError. Here's a test case that causes the JIT to crash with your patch: https://gist.github.com/swtaarrs/1fcac0647bfd90797608c12005076bbd.

tekknolagi commented 1 year ago

Ooh, interesting. How does that work in the JIT right now? I tried to construct some HIR with mismatched types and got an assertion failure in Simplify

facebook-github-bot commented 1 year ago

@tekknolagi has updated the pull request. You must reimport the pull request before landing.

tekknolagi commented 1 year ago

Should be fixed, but we still unfortunately have a dangling IsNegativeAndErrOccurred emitted.

swtaarrs commented 1 year ago

Were you hitting this one? That's the one my test hits with your patch when I run locally. If we didn't have that assertion, the test would probably fail when the function failed to raise.

hir::PrimitiveUnbox has a Type attached to it to indicate the target type. You need to check that the input to the PrimitiveBox instruction has a type such that it will never fail to unbox to the PrimitiveUnbox's type.

Your new commit that came in just now isn't quite right - you need to not perform the optimization when the types are incompatible, not force the types to fit with an IntConvert. I'm guessing that if you add exactly the test from my gist (which is worth doing), you'll see a behavioral difference between the JIT and the interpreter now.

tekknolagi commented 1 year ago

Good call. Updating now. Thanks!

facebook-github-bot commented 1 year ago

@tekknolagi has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot commented 1 year ago

@jbower-fb has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot commented 1 year ago

@tekknolagi has updated the pull request. You must reimport the pull request before landing.

tekknolagi commented 1 year ago

Fixed lint... sorry

facebook-github-bot commented 1 year ago

@jbower-fb has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot commented 1 year ago

@jbower-fb has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot commented 1 year ago

@jbower-fb merged this pull request in facebookincubator/cinder@dc03508ef03b7835b88f35d894195837f070f156.