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

Jump to bottom

C++: Don't generate Loads for constant expressions #14115

Merged
merged 4 commits into from Sep 1, 2023
Merged

C++: Don't generate Loads for constant expressions #14115

merged 4 commits into from Sep 1, 2023
+67 −10

Conversation

MathiasVP
Copy link
Contributor

No description provided.

@MathiasVP MathiasVP requested a review from a team as a code owner August 31, 2023 19:39
@github-actions github-actions bot added the C++ label Aug 31, 2023
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Aug 31, 2023
MathiasVP
MathiasVP commented Aug 31, 2023
View reviewed changes
cpp/ql/test/library-tests/ir/modulus-analysis/test.cpp
@@ -19,13 +19,11 @@ void m(int i, bool cond, int x, int y) {
mod(j); // $ mod=0,3,4

if (x % c1 == 3 && y % c1 == 7) {
// Need implies_v2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out this didn't actually need implies_v2 😂. Just proper IR!

andersfugmann
andersfugmann approved these changes Sep 1, 2023
View reviewed changes
Copy link
Contributor

@andersfugmann andersfugmann left a comment

Choose a reason for hiding this comment

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

LGTM.

@MathiasVP MathiasVP merged commit 67a0112 into github:main Sep 1, 2023
14 checks passed
@andersfugmann
Copy link
Contributor

andersfugmann commented Sep 1, 2023 •
edited

DCA query results looks good; solved 18 FN's and 2 FP's.

I think this also reduced IR inconsistencies on operandAcrossFunctions and nonUniqueEnclosingIRFunction, but introduced two more unnecessaryPhiInstructions on openzfs.

One experiment failed, but is seems to be an extraction failure.

@MathiasVP Is the changes for unnecessaryPhiInstructions ir concistency check on openzfs something to be concerned about?

@MathiasVP
Copy link
Contributor Author

MathiasVP commented Sep 1, 2023 •
edited

Indeed the DCA results look great 🎉.

I don't think the new inconsistencies is something to worry about. I'll see what they are just to be sure, though 👀

@MathiasVP
Copy link
Contributor Author

I don't think the new inconsistencies is something to worry about. I'll see what they are just to be sure, though 👀

Nothing to blame on this PR. We're simply just generating different IR for a function that's duplicated based on what looks like an ODR violation.

For those interested: The difference in IR is because we know realize that this branch is always true (because ntasks is equal to dmu_find_threads, and because of this PR we now correctly model the value of this constant integer and realize that it's always 0).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andersfugmann andersfugmann andersfugmann approved these changes

Assignees
No one assigned
Labels
C++ no-change-note-required This PR does not need a change note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@MathiasVP @andersfugmann