Experiment to improve code readability in CodegenXtensa #7471
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is just a draft because (1) it's not complete, and (2) I'm not 100% sure this is the best improvement to make. The problem I wanted to solve here was that CodeGenXtensa currently has a pervasive pattern of repeated-if-else code to choose the right type, and repeated (redundant) code inside each else clause. I was hoping that something enabling a more `switch`-like chunk of code would be smaller, easier to read and reason about, and have less redundant chunks (e.g. calls to `print_assignment()` for every type clause which are identical except for the instruction name). This takes the approach that we can use a map to make `halide_type_t` -> a bespoke enum; if the type is in the map, it means it's equivalent to `is_native_xtensa_vector<>()` returning true. This allows us to use `switch` statements in many places with only a slight overhead, but (maybe?) simpler-to-read code; when combined with judicious use of lambdas, we (hopefully?) get less redundant code. That said... this still isn't quite as elegant / easy to read as I'd like; whether or not this is enough of an improvement to be worth finishing is a matter I'd like some other folks thoughts on. Alternately, if someone has an even-better idea for handling all this, I'd love to hear anout it :-)
steven-johnson
added a commit
that referenced
this pull request
Apr 1, 2023
We currently re-create this map every time `print_xtensa_call()` is called, which is silly. Let's create it once per instance and keep it in a member variables. Also, an unordered_map is likely to be more efficient for our purposes here. (Harvested from #7471)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is just a draft because (1) it's not complete, and (2) I'm not 100% sure this is the best improvement to make.
The problem I wanted to solve here was that CodeGenXtensa currently has a pervasive pattern of repeated-if-else code to choose the right type, and repeated (redundant) code inside each else clause.
I was hoping that something enabling a more
switch-like chunk of code would be smaller, easier to read and reason about, and have less redundant chunks (e.g. calls toprint_assignment()for every type clause which are identical except for the instruction name).This takes the approach that we can use a map to make
halide_type_t-> a bespoke enum; if the type is in the map, it means it's equivalent tois_native_xtensa_vector<>()returning true. This allows us to useswitchstatements in many places with only a slight overhead, but (maybe?) simpler-to-read code; when combined with judicious use of lambdas, we (hopefully?) get less redundant code.That said... this still isn't quite as elegant / easy to read as I'd like; whether or not this is enough of an improvement to be worth finishing is a matter I'd like some other folks thoughts on.
Alternately, if someone has an even-better idea for handling all this, I'd love to hear anout it :-)