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

fold_divmod mwcc inner add fix #265

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

SeekyCt
Copy link
Contributor

@SeekyCt SeekyCt commented Aug 29, 2023

Currently fold_divmod's translation of (MULT_HI(x, N) + x) >> M --> MULT_HI(x, N) >> M assumes that x and N will be in that order inside of the MULT_HI, however that assumption fails for some MWCC code. This PR just allows for either argument to be equal to what's being added for this translation, which allows for a lot more of the divisions to be picked up on MWCC.

There's still a couple of other issues with MWCC for fold_divmod I'd like to try figure out too:

  • The division by 3 in test_s32_div seems to be resolving the ((u32) MULT_HI(0x55555556, sp8) >> 0x1FU) into (sp8 / 6442450941) early, which means that the outer error removal ((x / N) + ((x / N) >> 31)) --> x / N can't take place (the statement is (MULT_HI(0x55555556, sp8) + (sp8 / 6442450941)) by the time this runs). I'm not sure I understand the logic & flow of this enough to know how to fix this, so any advice there would be appreciated
  • The division by -3 in test_s32_div uses inner subtraction instead of inner addition, haven't looked into this one much yet so I'll try figure more out myself

@simonlindholm simonlindholm merged commit c57c426 into matt-kempster:master Aug 29, 2023
1 check passed
@simonlindholm
Copy link
Collaborator

The division by 3 in test_s32_div seems to be resolving the ((u32) MULT_HI(0x55555556, sp8) >> 0x1FU) into (sp8 / 6442450941) early, which means that the outer error removal ((x / N) + ((x / N) >> 31)) --> x / N can't take place (the statement is (MULT_HI(0x55555556, sp8) + (sp8 / 6442450941)) by the time this runs). I'm not sure I understand the logic & flow of this enough to know how to fix this, so any advice there would be appreciated

Hm. This can be pretty tricky, our pattern matching setup is wonky and not very flexible. In this particular case there's an easy out though: refuse to detect a division by more than 2^32.

@SeekyCt
Copy link
Contributor Author

SeekyCt commented Aug 30, 2023

Thanks, that does work to block it but made me realise the actual issue here is different. The true problem is that the final if statement converting mult_his into divisions was also assuming that right_expr would be the literal, but if the original_expr is the mult_hi then this isn't actually guaranteed for mwcc, just fixing that massively improved results. Will try get another PR ready soon.

@SeekyCt SeekyCt deleted the mwcc-divmod-fix branch August 30, 2023 15:28
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.

2 participants