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

Fixed bug that Saturating operations on Index 1 were unsafe (#682). #686

Merged
merged 1 commit into from
Jul 29, 2019

Conversation

rowanG077
Copy link
Member

@rowanG077 rowanG077 commented Jul 26, 2019

I have added constant folding test for the specific case Index 1 case.

The parser doesn't detect yet whether it is actually constant folded since it only will trigger an error when it finds a specific number range. It's not possible to easily extend this with the correct number since for Index 1 all operations take and result in 0.

Creating a parser to check that no real operations are taking place would be quite an undertaking I think. What do you think @martijnbastiaan?

@rowanG077 rowanG077 force-pushed the master branch 5 times, most recently from 38add0c to 3ba4c75 Compare July 26, 2019 16:56
@martijnbastiaan
Copy link
Member

Hey @rowanG077, thanks for your second PR :)!

I think the following would suffice:

  case (satAdd @(Index 1) SatWrap (lit 0) (lit 0)) of {0 -> 0; 1 -> 22270 :: Int}
  case (satSub @(Index 1) SatWrap (lit 0) (lit 0)) of {0 -> 0; 1 -> 22271 :: Int}
  ...

If Clash successfully constant folds this, you shouldn't see 22270 in the output. It might be that it doesn't actually constant fold this, but that it strips out the case entirely due to Index 1 being zero bits. I'm a bit fuzzy on the details, as it has been a while since I looked at this, but either way should be fine.

@rowanG077 rowanG077 force-pushed the master branch 2 times, most recently from a765fce to 17fd43e Compare July 26, 2019 20:10
@rowanG077 rowanG077 changed the title WIP: Fixed bug that Saturating operations on Index 1 were unsafe (#682). Fixed bug that Saturating operations on Index 1 were unsafe (#682). Jul 26, 2019
@martijnbastiaan
Copy link
Member

LGTM, thanks!

@rowanG077 rowanG077 force-pushed the master branch 2 times, most recently from d165617 to 3d59c8c Compare July 28, 2019 14:07
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

3 participants