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

Control Dependence Analysis Pass #98

Closed
wants to merge 55 commits into from
Closed

Conversation

AyaElAkhras
Copy link
Member

Added a ControlDependenceAnalysis pass and instantiated it in a Transform pass called FuncSSAToGSA

AyaElAkhras and others added 30 commits June 10, 2024 23:18
@AyaElAkhras
Copy link
Member Author

Hi Jiahui, thanks again for your feedback. I accounted for most of your suggestions and marked them as resolved :) I left a comment for the suggestions of replacing vectors with sets above. I'm missing a few more comments that I left unresolved but will try to come back to later...

@lucas-rami
Copy link
Member

Is this ready for review? If so then conflicts with main need to be resolved. If a smaller contribution that does not conflict can be extracted from the current state of the branch (e.g., just the new analysis) then feel free to remove the other stuff from this branch.

@lucas-rami
Copy link
Member

Conflicts with main need to be resolved before I review this.

@AyaElAkhras
Copy link
Member Author

I deleted conflicting files and limited the contribution of this PR to one analysis pass that adds a source file (ControlDependenceAnalysis.cpp), a header file (ControlDependenceAnalysis.h) and the updated CMakeLists.txt

Copy link
Member

@lucas-rami lucas-rami left a comment

Choose a reason for hiding this comment

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

Left a bunch of comments, mostly cosmetics/optimizations except a few on data representation. Nothing that should be too long to address though.

As @Jiahui17 pointed out initially, the code is not clang-tidyed, please install the necessary tool and run it on your code (VSCode extension).

tutorials/Introduction/Ch1/frontend-script.dyn Outdated Show resolved Hide resolved
tutorials/Introduction/Ch1/loop_multiply.c Outdated Show resolved Hide resolved
include/dynamatic/Analysis/ControlDependenceAnalysis.h Outdated Show resolved Hide resolved
include/dynamatic/Analysis/ControlDependenceAnalysis.h Outdated Show resolved Hide resolved
lib/Analysis/ControlDependenceAnalysis.cpp Outdated Show resolved Hide resolved
lib/Analysis/ControlDependenceAnalysis.cpp Outdated Show resolved Hide resolved
lib/Analysis/ControlDependenceAnalysis.cpp Outdated Show resolved Hide resolved
lib/Analysis/ControlDependenceAnalysis.cpp Outdated Show resolved Hide resolved
include/dynamatic/Analysis/ControlDependenceAnalysis.h Outdated Show resolved Hide resolved
@AyaElAkhras
Copy link
Member Author

Accounted for the cosmetic stuff and left one comment about deleting the getter functions. Will get to the rest of the comments later.

@lucas-rami
Copy link
Member

lucas-rami commented Aug 26, 2024

I realize that Jiahui pointed out the vector to set thing as well and that you were worried that doing so would prevent you from easily modifying elements in your data-structure. It's in fact easier to modify in a set than a vector as well as being O(1) instead of O(n) in general, see example below.

mlir::DenseSet<Block*> blocks;
// ... do something to the set ...

// Removing
Block* toRemove = ...;
if (blocks.erase(toRemove)) {
  // The removed block was part of the set and was removed
} else {
  // The removed block was *not* part of the set, nothing happened
}

// Inserting
Block* toInsert = ...;
if (auto [_, newElem] = blocks.insert(toInsert); newElem) {
  // The inserted block was not part of the set and was inserted
} else {
  // The inserted block was *already* part of the set, nothing happened
}

@AyaElAkhras
Copy link
Member Author

Accounted for all comments and retested the implementation. There are some remaining warnings from clang-tidy, but I do not have time to resolve them now. It seems to me that they have to do with variable names--will check later.

@lucas-rami
Copy link
Member

Where does this stand w.r.t. #177? Is this stale and should be closed because it will reimplemented differently? Or should it just be pushed to the finish line?

@pcineverdies
Copy link
Collaborator

You are right, when dealing with #177 we did not think about this analysis pass. Since a few modifications have been done in the meanwhile, I am going to open a new PR accordingly.

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.

4 participants