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

partialexecuter: increment and check basicblock visit count after terminals #205

Closed
wants to merge 2 commits into from

Conversation

Hyxogen
Copy link
Contributor

@Hyxogen Hyxogen commented Feb 7, 2024

No description provided.

@Hyxogen Hyxogen requested a review from yuri91 February 7, 2024 10:36
@Hyxogen
Copy link
Contributor Author

Hyxogen commented Feb 7, 2024

Although the crash in leaningtech/cheerp-meta#133 doesn't seem to happen anymore, it did cause a hang.

Closes: leaningtech/cheerp-meta#133

Copy link
Member

@yuri91 yuri91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really have experience with PartialExecuter. I would like @alexp-sssup to have a look.

@yuri91 yuri91 requested a review from alexp-sssup February 7, 2024 10:40
else
{
skip = true;
if (data.getVisitCounter(next) < MAX_NUMBER_OF_VISITS_PER_BB)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks reasonable. Currently the check is only done when starting the visit from a new block, but internal visits are not accounted for. Two things

  1. Move the new check inside the existing if to better preserve the code history
  2. Please validate that prints is still correctly optimized away in easy cases, we might need to bump the max number of visits since we are now counting much more often

@Hyxogen
Copy link
Contributor Author

Hyxogen commented Feb 8, 2024

@alexp-sssup I did a check with the following code and it generated the exact same before and after the changes:

#include <stdio.h>

int main() {
        printf("%3i, %x, %f, %e, %f, %g, %0f, %10f, %.10f, %-f\n", 42, 21, 3.41, 2.72, 0.5, -6.12, 6564.34534, 0.001, 3.0, -10.433);
}

@alexp-sssup
Copy link
Member

That test case is not a good one since it's keeping in lots of logic by using multiple type of % format options.

Try a simpler test case with just %i or %x

@Hyxogen
Copy link
Contributor Author

Hyxogen commented Feb 8, 2024

That test case is not a good one since it's keeping in lots of logic by using multiple type of % format options.

Try a simpler test case with just %i or %x

I also tried using this one, which also generated the same code before and after:

#include <stdio.h>

int main() {
        printf("%i %i %i %i %i\n", 42, 21, 0, 1, 543234);
        printf("%x %x %x %x %x\n", 42, 21, 0, 1, 543234);
}

@Hyxogen
Copy link
Contributor Author

Hyxogen commented Feb 8, 2024

That test case is not a good one since it's keeping in lots of logic by using multiple type of % format options.
Try a simpler test case with just %i or %x

I also tried using this one, which also generated the same code before and after:

#include <stdio.h>

int main() {
        printf("%i %i %i %i %i\n", 42, 21, 0, 1, 543234);
        printf("%x %x %x %x %x\n", 42, 21, 0, 1, 543234);
}

I checked, and before and after it does not go through this branch that handles terminals though

@alexp-sssup
Copy link
Member

Merged

@alexp-sssup alexp-sssup closed this Feb 8, 2024
@alexp-sssup
Copy link
Member

I checked, and before and after it does not go through this branch that handles terminals though

That's ok, it means this code path is not as critical as I though

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