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

Brodes/guard flow parsing k #17933

Closed
wants to merge 16 commits into from

Conversation

bdrodes
Copy link
Contributor

@bdrodes bdrodes commented Nov 7, 2024

No description provided.

@github-actions github-actions bot added the C++ label Nov 7, 2024
…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.
…based integer extraction. Updated int_val to handle conversions.
Comment on lines 1390 to 1395
private int int_value(Instruction i) {
result = valueNumber(i).getAnInstruction().(IntegerOrPointerConstantInstruction).getValue().toInt()
result =
valueNumber(i).getAnInstruction().(IntegerOrPointerConstantInstruction).getValue().toInt()
or
// handle conversions
result = int_value(valueNumber(i).getAnInstruction().(ConvertInstruction).getUnary())
Copy link
Contributor

Choose a reason for hiding this comment

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

Watch out with materializing all Instructions when all we really care about is the value number. If we instead change the i parameter to be a ValueNumber instead this predicate will be a lot smaller.

Note that this will require changing all the callers so that they convert the argument to a value number. You can define an (inline) helper predicate to do that conversion for you. This will keep all callers as-is, and still avoid unnecessary materialization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, but what about inlining int_value instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can't inline it since it's recursive, unfortunately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah true true. The inline helper function I think is the winner then, which is basically the best of both worlds.

Copy link

@Sexypella Sexypella left a comment

Choose a reason for hiding this comment

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

microsoft:brodes/guard_flow_parsing_k

@bdrodes
Copy link
Contributor Author

bdrodes commented Nov 14, 2024

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.

@bdrodes bdrodes closed this Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants