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

C++: Add another IR consistency query #18013

Merged
merged 4 commits into from
Nov 18, 2024

Conversation

MathiasVP
Copy link
Contributor

As discussed async, in the guards library we currently have special logic to handle the fact that C databases don’t have int-to-bool conversions in stuff like:

int x = ...
if(x) {
  ...
}

because of those missing conversions, the IR simply branches on the load of x instead of the result of a CompareNEInstruction. While this makes sense since some versions of C doesn't have a Boolean type. However, it does mean that we have to special-case this in the guards library (for example, the inNonZeroCase column in #16533).

I now realize, however, that it’s probably simpler to just add those CompareNEInstructions as part of IR generation. Then we don’t need that complexity in the guards library at all, and we don’t need to think about whether we’re in C or in C++ in the library.

The IR changes itself are pretty trivial, but in order to get a feeling for whether they work on real-world databases I'm first adding a new consistency check.

@MathiasVP MathiasVP requested a review from a team as a code owner November 18, 2024 15:29
@github-actions github-actions bot added the C++ label Nov 18, 2024
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Nov 18, 2024
dbartol
dbartol previously approved these changes Nov 18, 2024
Copy link
Contributor

@dbartol dbartol left a comment

Choose a reason for hiding this comment

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

LGTM.

Skimming over the results from the new consistency query, they seem to be all in C code, which is what we expected.

jketema
jketema previously approved these changes Nov 18, 2024
Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

LGTM. Do the syntax-zoo test results also need to be updated?

Comment on lines +553 to +560
exists(Instruction unary |
unary = instr.(LogicalNotInstruction).getUnary() and
not unary.getResultIRType() instanceof IRBooleanType and
irFunc = getInstructionIRFunction(instr, irFuncText) and
message =
"Logical Not instruction " + instr.toString() +
" with non-Boolean operand, in function '$@'."
)
Copy link
Contributor

@jketema jketema Nov 18, 2024

Choose a reason for hiding this comment

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

I'm not sure to what extent you want to change the IR behavior here, but this probably requires some care with cases like the following (for which you have already added a test):

int y = ...;
int x = !y

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I intend to fix those cases as well 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

For !y, I'd suggest doing the same thing we do for C++, which is to first convert the int to bool via CompareNE.

Separate from the consistency issue, I wonder if translating !y directly as CompareEQ, instead of CompareNE, LogicalNot, would give better results somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Separate from the consistency issue, I wonder if translating !y directly as CompareEQ, instead of CompareNE, LogicalNot, would give better results somewhere.

The problem with this is the translation back to Exprs (which we need in particular when someone does dataflow to a condition of an if statement). In that case, the unconverted result expression should be the CompareNE, and the converted result expression should be the LogicalNot. But if we merge them together as a CompareEQ then the unconverted result expression of !y would be a CompareEQ which is a bit strange.

@MathiasVP MathiasVP dismissed stale reviews from jketema and dbartol via ccca0b6 November 18, 2024 15:56
@MathiasVP MathiasVP merged commit f2f83f7 into github:main Nov 18, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants