EPFL-LAP / dynamatic

DHLS (Dynamic High-Level Synthesis) compiler based on MLIR
Other
60 stars 18 forks source link

Control Dependence Analysis Pass #98

Open AyaElAkhras opened 3 months ago

AyaElAkhras commented 3 months ago

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

Jiahui17 commented 3 months ago

Hiii! Thanks for the contribution! Here I directly quote the comments that @lucas-rami wrote in another PR:

Please follow our contribution guidelines. In particular, set up and run clang-format/clang-tidy on your code and fix any warning that pops up. The PR needs a description (this doesn't have to be long, just a description of the contribution's content that I can base myself on to make the future commit message). See past PRs for examples.

AyaElAkhras commented 3 months ago

Thanks @Jiahui17 for your feedback :)

Jiahui17 commented 3 months ago

In the description of the PR, it would be nice to provide some way to run the changed parts. For instance, here is a bash script that runs your pass on one of the benchmarks in integration-test:

#!/bin/bash

# Call this script in the dynamatic directory

echo "set-dynamatic-path .; \
  set-src ./integration-test/fir/fir.c; \
  compile --simple-buffers; \
  exit" \
  | bin/dynamatic --exit-on-failure --debug

./bin/dynamatic-opt \
  ./integration-test/fir/out/comp/std_transformed.mlir \
  --func-ssa-to-gsa \
  --debug
AyaElAkhras commented 3 months ago

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 commented 2 months ago

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 commented 2 months ago

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

AyaElAkhras commented 2 months ago

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

AyaElAkhras commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.