Brodes/guard flow parsing #17907
Brodes/guard flow parsing #17907
Conversation
…boolean flow logic is completed.
…hanges made to guards (baseline expected file).
…instruction if one exists.
…eriving instruction if one exists." This reverts commit d17dee5.
| result = valueNumber(instr).getAnInstruction() and | ||
| result.toString() != instr.toString() and | ||
| result instanceof CompareInstruction |
There was a problem hiding this comment.
We don't want to use toString when implementing query/library logic. toString output is being changed consistently to provide better UX in query output without any deprecation periods, and there shouldn't be any hidden invariants in the output of these predicates. So we need to come up with something else here.
What's the job of this predicate really? I think you're trying to find an equivalent instruction that performs a comparison so that we can derive guards from that comparison instead of the original instruction, right? But why should we restrict that to be CompareInstructions only? And why check that they have a different toString? If it's because we want to not have result = instr in this case could we not simply write result != instr?
And related to the QLDoc: What do you mean by "define" instr? I think we should have an example in the QLDoc to motivate this predicate.
There was a problem hiding this comment.
I'll take the discussion offline. But some quick comments:
- Agreed on tostring, but I don't know a better approach, we will discuss
- I can't say result != instr because that's not what I'm trying to achieve here. I'm trying to not get other GVN instructions that are identical. e.g.,
a->foo->barI don't need to get every a->foo->bar, I need to get the instruction that defines a->foo->bar and ignore everything else. I'm playing a game with GVN to give me the SSA def. We probably need to use SSA instead, but this was a quick demonstration of how to get what I wanted (it at least works with the AST version of this in my tests in the past) - I only limited CompareInstruction because I thought I saw you were doing the same, but I've been testing with and without it.
- The TLDR for me is either we use SSA, or keep this kind of approach but use the IR to help me find only the GVN with the definition. I'm no expert on the syntax of all the different IR instruction types, but perhaps you can provide some guidance.
Taking further discussion into a DM until we settle on a few things.
…l uses of getDerivedInstruction to occur in 'base cases", specially only the compares_lt compares_eq and unary_compares_eq predicates. The premise is that users can modify/add any other logic without having to know about getDerivedInstruction; however, any updates to the compares_lt/eq predicates are the only place where the derived instruction must be used.
| result = instr | ||
| // Special handling for conversions | ||
| result = | ||
| getDerivedInstruction(valueNumber(instr).getAnInstruction().(CompareNEInstruction).getLeft()) |
There was a problem hiding this comment.
You should probably also check that the right-hand side has an int_value of 0 to make sure this is actually an int-to-bool conversion.
I also think this deserves a more descriptive comment than what is currently here.
There was a problem hiding this comment.
Can you explain that a bit? What does a value of 0 or non-zero mean?
| ( | ||
| exists(BinaryLogicalOperation e | e.getAnOperand() = test.getUnconvertedResultExpression()) | ||
| or | ||
| exists(UnaryLogicalOperation e | e.getAnOperand() = test.getUnconvertedResultExpression()) | ||
| or | ||
| exists(IfStmt i | i.getCondition() = test.getUnconvertedResultExpression()) | ||
| or | ||
| exists(Loop i | i.getCondition() = test.getUnconvertedResultExpression()) | ||
| or | ||
| exists(ConditionalExpr c | c.getCondition() = test.getUnconvertedResultExpression()) | ||
| // Todo recursive conditionalExpr in guard |
There was a problem hiding this comment.
I don't think we want to do the pruning like this. We really want to avoid depending on the specific syntax this deep inside the libraries.
And as you say yourself: This also still misses some cases. Once this is done properly and completely, we can avoid these ad-hoc conjuncts below:
not test.isGLValue() and
not simple_comparison_eq(test, _, _, _, _) and
not simple_comparison_lt(test, _, _, _) and
not test = any(SwitchInstruction switch).getExpression() and
(
test.getResultIRType() instanceof IRAddressType or
test.getResultIRType() instanceof IRIntegerType or
test.getResultIRType() instanceof IRBooleanType
)
|
Worked with @MathiasVP on a prior solution of his to find inefficiencies and worked out conversion concerns. We are going to take the lessons learned here and put them into that PR. Closing this draft. |
No description provided.