theta: change API for mapping loop variables #675
Conversation
|
This change again provides names for variable roles that may be controversial:
|
There was a problem hiding this comment.
Looks all good to me. I am fine with the names, even though preIteration and postIteration would be a bit more descriptive, but is also quite long.
The only reason why I block this temporarily is because of the loop var removal and index remapping. Where is this happening such that this is necessary?
| class ThetaInput; | ||
| class ThetaOutput; | ||
| class ThetaResult; |
There was a problem hiding this comment.
You just removed these classes below. These forward declarations can be removed as well, no?
| LoopVar | ||
| AddLoopVar(rvsdg::output * origin); |
There was a problem hiding this comment.
Could we get some documentation on it?
| /* theta node */ | ||
|
|
There was a problem hiding this comment.
Can be removed. Does not add any value
jlm/rvsdg/theta.cpp
Outdated
| auto thetaInput = util::AssertedCast<ThetaInput>(input); | ||
| return ThetaArgument::Create(region, *thetaInput); | ||
| } | ||
| auto nf = graph()->node_normal_form(typeid(jlm::rvsdg::operation)); |
| // Calling RemoveThetaInputsWhere/RemoveThetaOutputsWhere can result | ||
| // in inputs (and pre-loop arguments) and outputs (and post-loop results) | ||
| // to become unmatched. In this case, the theta node itself has | ||
| // "invalid" shape until fixed properly. | ||
| // The indices of unmatched inputs/outputs are tracked here to | ||
| // detect this situation, and also to provide correct mapping. | ||
| // Computing the mapping is a bit fiddly as it requires adjusting | ||
| // indices accordingly, should seriously consider whether this | ||
| // is really necessary or things can rather be reformulated such that | ||
| // inputs/outputs are always consistent. | ||
|
|
||
| std::optional<std::size_t> | ||
| MapInputToOutputIndex(std::size_t index) const noexcept; | ||
|
|
||
| std::optional<std::size_t> | ||
| MapOutputToInputIndex(std::size_t index) const noexcept; | ||
|
|
||
| void | ||
| MarkInputIndexErased(std::size_t index) noexcept; | ||
|
|
||
| void | ||
| MarkOutputIndexErased(std::size_t index) noexcept; | ||
|
|
||
| std::vector<std::size_t> unmatchedInputs; | ||
| std::vector<std::size_t> unmatchedOutputs; |
There was a problem hiding this comment.
Is this really necessary? The only places where theta variables are removed should be DNE, and there we use the RemoveThetaInputsWhere and RemoveThetaOutputsWhere immediately after each other to ensure that the inconsistent state is very local. Where else are theta inputs/outputs are removed such that this becomes necessary?
There was a problem hiding this comment.
I don't like this, and I have a proposal how to do it better, but I would like to separate this from the present PR:
Outputs that are deemed to be "dead" do not play a role in computation, so we are allowed to assign an arbitrary value to it. So instead of leaving unbalanced inputs/outputs we can always route the pre-loop value through to satisfy it. Then, in a second step, we can eliminate them as loop-invariant, and unused inside the loop. This way we avoid ever having unbalanced inputs/outputs, but the tracking of trying to eliminate them needs to move into DNE.
There was a problem hiding this comment.
@caleridas Sounds reasonable to me. I will mark this conversation as resolved then. I would like you to push this fix for the imbalance then rather soon though such that we do not have the code above lingering around for too long.
Provide an API to loop nodes that allows mapping the various
representation pieces of a loop variable. Remove
Theta{Input|Output|Argument|Result}.
|
I have a hard time interpreting the failure from the hls test suite. On github I have no clear indication what exactly fails and when I run it locally, I get this: Building: base/test_return While I do not understand why my change causes semantic change to the compilation pipeline, I am a bit wondering whether this causes a "semantically meaningful" change to the compilate, or whether the change is fine and the test is a bit too change-sensitive. How would I distinguish between these? @sjalander @phate |
|
I tried pushing a change to hls-test-suite: --- a/src/base/test_matrix.cycles but I lack permission to create a branch on that repository. |
|
@caleridas I invited you the llvm-test-suite repository and the hls-test-suite repository as collaborator. I leave the issues with the HLS test suite to @sjalander to answer, he knows way more about it than I do. |
Provide an API to loop nodes that allows mapping the various representation pieces of a loop variable. Remove
Theta{Input|Output|Argument|Result}.