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

Add more tests and KillInst() to DefUseManager #347

Closed
wants to merge 4 commits into from

Conversation

Qining
Copy link
Contributor

@Qining Qining commented Aug 13, 2016

Add KillInst() so that def-use manager can handle instrutions that do
not have result ids.

Add ClearDef() and ClearInst() two private functions.

Add tests for AnalyzeInstDefUse().

Add tests for KillInst().

Add a test that call AnalyzeDefUse() on two different modules.

@Qining Qining force-pushed the update-def-use-mgr branch from 915b0b1 to ac73401 Compare August 13, 2016 20:52
// false;
bool ShouldRecord(const ir::Operand& operand) const;

// Erases the record that: instruction |user| uses id |used_id|.
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove the ":"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 4d7c019

@dneto0
Copy link
Collaborator

dneto0 commented Aug 15, 2016

-1
Tests look ok.
Several issues with the code need addressing.

Qining added 4 commits August 16, 2016 11:10
Add KillInst() so that def-use manager can handle instrutions that do
not have result ids.

Add ClearInst() and ClearDef() private functions.

Add tests for AnalyzeInstDefUse().

Add tests for KillInst().

Add a test that call AnalyzeDefUse() on two different modules.
Fix some naming issues.
Refactor the ClearInst() and ClearDef() to simplify the code.
Remove unnecessary commas in the test cases.
@Qining
Copy link
Contributor Author

Qining commented Aug 16, 2016

Close as it is moved to #356

@Qining Qining closed this Aug 16, 2016
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