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

Restrict input bits for analysis implementations in the presence of PC #659

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

manasij7479
Copy link
Collaborator

No description provided.

@regehr
Copy link
Collaborator

regehr commented Nov 18, 2019

I need more here. what's going on? why is it correct?

@regehr
Copy link
Collaborator

regehr commented Nov 18, 2019

add comments in the code explaining the strategy and rationale for correctness.

@regehr
Copy link
Collaborator

regehr commented Nov 18, 2019

remove commented-out lines of code unless they serve a specific purpose, in which case explain them

@regehr
Copy link
Collaborator

regehr commented Nov 18, 2019

ping

@manasij7479
Copy link
Collaborator Author

Oh, forgot about this.
Will rebase, add comments for explanation and update.

@manasij7479
Copy link
Collaborator Author

updated

@@ -119,6 +119,32 @@ std::pair<llvm::APInt, llvm::APInt> knownBitsNarrowing
return {KnownNotZero, KnownNotOne};
}

std::unordered_map<Inst *, llvm::APInt>
PruningManager::computeInputRestrictions() {
std::unordered_map<Inst *, std::pair<llvm::APInt, llvm::APInt>> Seen;
Copy link
Collaborator

Choose a reason for hiding this comment

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

you don't need to change it here but overall you should avoid using a std::pair for same-typed things like this, and instead pick an implementation which assigns appropriate names to things. here two separate variables would work, or else a struct with members called Zero and One or similar

lib/Infer/Pruning.cpp Outdated Show resolved Hide resolved
lib/Infer/Pruning.cpp Outdated Show resolved Hide resolved
@regehr
Copy link
Collaborator

regehr commented Nov 19, 2019

I still have no idea why this does what you want it to do, you'll have to explain it more either in the code or in person

@regehr
Copy link
Collaborator

regehr commented Dec 8, 2019

@manasij7479 this is one of several things you need to get back to and finish up

@manasij7479
Copy link
Collaborator Author

Addressed the comments, not sure what to do about the explanation.

@regehr
Copy link
Collaborator

regehr commented Dec 10, 2019

how about some examples which show this working, then?

@regehr
Copy link
Collaborator

regehr commented Dec 10, 2019

I don't like random, complicated code which is neither tested nor well-explained. I know you can do better than this, please make an effort here.

@regehr
Copy link
Collaborator

regehr commented Dec 10, 2019

it seems pretty obvious what to do here:

  • add restricted bits to souper-interpret
  • write test cases checking for sound processing of souper expressions with pcs

@regehr
Copy link
Collaborator

regehr commented Dec 10, 2019

I have no idea why you would be confident enough in this code's correctness to submit it for inclusion in souper without having done this

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.

2 participants