Region Sequentializer #839
Conversation
| const util::HashSet<const rvsdg::Node *> & visited); | ||
|
|
||
| size_t CurrentSequentialization_ = 0; | ||
| std::vector<Sequentialization> Sequentializations_; |
There was a problem hiding this comment.
Yes, I am aware that this is exponential in space. However, I would like to start with this first.
There was a problem hiding this comment.
That is fine. Looking at the implementation, it should be quite easy to make it linear in space by just storing the sequence, and possibly a nodeIteratiorIndex for each node.
| return 0; | ||
| } | ||
|
|
||
| JLM_UNIT_TEST_REGISTER("jlm/llvm/backend/ExhaustiveSequentializerTests-Test", Test) |
There was a problem hiding this comment.
I will implement proper unit tests once the region sequentializer interface is settled etc.
There was a problem hiding this comment.
Overall the design looks good and extensible. My only issue is with the API for iterating over sequentializations, as it is hard to keep track of when sequentializations start looping (silently when requesting a sequentialization).
Could we instead have a design where a sequentializer's GetSequentialization() always gives a valid sequentialization, and ComputeNextSequentialization() intead starts returning false when it is no longer changing the current sequentialization.
While it might be nice in some cases to query if there exist more sequentializations, without moving on to the next one, that does make it a bit harder for implementations to implement the interface. My suggested interface would instead be used like
do {
auto & seq= sequentializer.GetSequentialization();
} while(sequentializer.ComputeNextSequentialization())| ExhaustiveRegionSequentializer::GetSequentialization() | ||
| { | ||
| if (!HasMoreSequentializations()) | ||
| ComputeNextSequentialization(); |
There was a problem hiding this comment.
I do not understand this code. Why can it not just return Sequentializations_[CurrentSequentialization_]. This code will effectively reset CurrentSequentialization_ to 0 if we are at the last sequentialization?
| if (HasMoreSequentializations()) | ||
| CurrentSequentialization_++; | ||
| else | ||
| CurrentSequentialization_ = 0; |
There was a problem hiding this comment.
I think this looping behavior is a bit surprising. It sort of allows overflow, but then automatically fixes the overflow again once you actually call GetSequentialization().
The way it is currently, you have to query HasMoreSequentialization() after ComputeNextSequentialization(), to find out that it actually didn't have a "next sequentialization".
I feel like that naming is a bit backwards, as I would expect to call HasMoreSequentializations() before calling ComputeNextSequentialization().
If would consider remaning HasMoreSequentializations() to just HasSequentialization().
| if (sequentializedNodes.size() == region.nnodes()) | ||
| { | ||
| Sequentializations_.emplace_back(sequentializedNodes); | ||
| } |
There was a problem hiding this comment.
This if should not be inside the loop, no?
It should instead be at the very top of this function, and early return if the condition is true?
| { | ||
| for (auto & node : region.Nodes()) | ||
| { | ||
| if (AllPredecessorsVisited(node, visited) && !visited.Contains(&node)) |
There was a problem hiding this comment.
I would swap the order of the conditions, to skip visited nodes more quickly
| const util::HashSet<const rvsdg::Node *> & visited); | ||
|
|
||
| size_t CurrentSequentialization_ = 0; | ||
| std::vector<Sequentialization> Sequentializations_; |
There was a problem hiding this comment.
That is fine. Looking at the implementation, it should be quite easy to make it linear in space by just storing the sequence, and possibly a nodeIteratiorIndex for each node.
| for (auto & [_, sequentializer] : Sequentializers_) | ||
| { | ||
| if (sequentializer->HasMoreSequentializations()) | ||
| return true; |
There was a problem hiding this comment.
Due to the looping behavior of the exhaustive sequentializer, I would be a bit worried about the RegionTreeSequentializer looping for a very long time, if it has two regions with, e.g., 11 and 10 sequentializations, it would run for 110 iterations before both sequentializers "line up" and have HasMoreSequentializations() return false.
| * sequentialization. | ||
| */ | ||
| virtual Sequentialization | ||
| GetSequentialization() = 0; |
There was a problem hiding this comment.
Could we make it return a const reference? If the user truly needs it to outlive the sequentializer they can always make a copy
This PR separates the sequentialization of the nodes from the LLVM converter. The reason for this change are as follows:
This draft is mean to get feedback. I will later split it up in proper PRs and clean it up.
@haved @caleridas