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

Add missing UnrolledLoopStatement to PGO region count calculator. #3524

Merged
merged 1 commit into from
Jul 30, 2020

Conversation

JohanEngelen
Copy link
Member

Also adds extra precautions to catch this problem earlier in the future.

Fixes issue #3375

@@ -0,0 +1,19 @@
// REQUIRES: PGO_RT
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this test get run with an LDC compiled with asserts enabled?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kinke do we have a CI system that runs with asserts enabled? I thought that the builds that output our latest-ci binaries do have asserts enabled, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, Azure (excl. Android) and Shippable feature enabled assertions.

Copy link
Contributor

Choose a reason for hiding this comment

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

With this addressed, all looks good to me.

// If this assert fails, a new statement type was added to the frontend, and
// thus we need to decide on how to handle PGO calculations for that, both
// in MapRegionCounters and in ComputeRegionCounts.
assert(0 && "All statement types should be explicitly handled to avoid "
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to do something in addition or other than assert so that detection is explicit in release mode builds that end users will use?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure. The error may not be user-actionable, and things may also just work without change needed. Would you suggest a warning saying that "Statement on line x is not supported by PGO" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

(note that this is the same for CaseRangeStatement)

Copy link
Contributor

@jondegenhardt jondegenhardt Jul 26, 2020

Choose a reason for hiding this comment

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

Would you suggest a warning saying that "Statement on line x is not supported by PGO" ?

Something like that. Perhaps requesting that an issue be filed for the purpose of improving LDC. This might make more sense if similar things are already being done elsewhere in the code base.

This rationale is that it might be difficult to catch these cases in the context of LDC development, and instead they'd be more likely to occur when running against user code. Do you think this is the case? If not, then such a message is not needed.

The other part is that PGO computation should proceed reasonably even if the assert condition arises. Ie. Not crash. I didn't try to review the code from this angle.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mind if I disregard this? We have many asserts in the codebase where we don't really report it to the user. (I think it is not very valuable for the user, to be honest) I do agree that the assert is not very likely to catch things during development, but it might when people use the beta build or latest-ci to have asserts enabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mind if I disregard this?

I don't mind. It was a suggestion for your consideration, nothing else. And it makes complete sense to be consistent with similar cases in the LDC code base.

@JohanEngelen
Copy link
Member Author

JohanEngelen commented Jul 26, 2020

Although it fixes the crash, it is not a correct fix and will result in wrong execution count numbers. Working on better fix.
I've implemented the correct fix now, and the testcase verifies the execution count for some more complex control flow.

…on and calculation. Also add extra precautions to catch this problem earlier in the future.

Fixes issue #3375
@jondegenhardt
Copy link
Contributor

I tested the PR on all the different reduced cases of bug #3375 I put together, the tool in tsv-utils where the problem first arose, and the full set of tsv-utils tools. Everything works fine.

@JohanEngelen
Copy link
Member Author

I guess the Azure failure is a random error? Only debuginfo/scopes_cdb.d Lit test fails, but this PGO code change has no impact on that test at all.

@JohanEngelen JohanEngelen merged commit e628a0a into master Jul 30, 2020
@JohanEngelen JohanEngelen deleted the pgocrashfix branch July 30, 2020 18:59
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