Am Dienstag, den 01.05.2018, 12:38 +0200 schrieb Nicolai Hähnle:
> First some high-level remarks:
>
> Why is `finalized` necessary? The `finalize` operation should be
> idempotent, i.e. if you call if twice in a row, the second time
> should be a no-op.
It actually works exactly like this.
> So you can just call finalize on the target array unconditionally.
> That would make the code cleaner.
The reason to call it conditionally is to avoid an unnecessary call,
the alternative would be an early return instead of the assert in
finalize_mappings().
In detail: In the way this piece of code is written right now the
`finalize` operation is called recursively, i.e. for each array mapping
the function is called, and if it needs to be mapped, the target array
is mapped first etc. Since the finalization of arrays is called in the
order of array indices the `finalize` may indeed be called various
times for one array.
I would have liked to get rid of this recursion, but I haven't found a
good way to do it. I tried evaluating the re-swizzling information on
the fly, but (with your notation from below) if in
merge_arrays(dst1, src1);
merge_arrays(dst2, dst1);
the second merge re-swizzles dst1, src1 also needs to re-swizzle. Given
that it is also possible to have
merge_arrays(dst1, src1);
merge_arrays(dst1, src2);
merge_arrays(dst1, src3);
merge_arrays(dst2, dst1);
keeping a reference to src* in dst1 is not very convenient, so IMHO it
is better to resolve it all at the end.
>
> Similarly, the `reswizzle` bit seems a bit redundant. Just check
> whether the array was remapped somewhere else -- if it wasn't,
> there's no need for swizzling, and if it was, the chances that the
> components stayed in the same place are miniscule (and you don't
> currently check for that anyway).
reswizzle is set when I interleave array (i.e. the third constructor),
otherwise, when the mapping is just a temporal merge (like the register
merge) then it is not set. Whether one or the other is more likely I
don't know. Anyway, given that I later build a swizzling map when there
is none, I agree that I should get rid of it.
> Also, why have two separate swizzle arrays? Just keep a single array
> (which can actually be an int8_t...) and use it for everything.
It was easier to distinguish between the two, but, yes I can and will
get rid of them.
> I also think that it would be better and less surprising to change
> how you think about the lifetime of the objects representing
> arrays. Basically, it should work like in a union-find structure:
> you have a single constructor which initializes the array to
>
> a) not be mapped anywhere else, and
> b) correctly initializes the component use bitmask
>
> Then you have a merge function
>
> merge_arrays(dst, src)
>
> or possibly a method
>
> dst.merge_arrays(src)
>
> (although with the way you're using indices instead of pointers, I'd
> prefer a standalone function) that
>
> a) updates the dst array's component use bitmask, and
> b) updates the src array's target pointer/index and swizzle
and c) updates the dst array's component live range
I'll see how I can refactor it to follow more your suggestion. I think
my code it actually not too far from this.
>
> This would also allow you to get rid of the duplicate access masks.
> In the end, the only data you really need per-array is:
>
> unsigned target_id;
> int8_t swizzle[4];
> uint8_t access_mask;
>
> I have some further comments, mostly about style, below.
Thanks for the review,
Gert
>
>
> On 28.04.2018 21:30, Gert Wollny wrote:
> > Signed-off-by: Gert Wollny <gw.foss...@gmail.com>
> > ---
> > src/mesa/Makefile.sources | 2 +
> > src/mesa/meson.build | 2 +
> > .../state_tracker/st_glsl_to_tgsi_array_merge.cpp | 283
> > +++++++++++++++++++++
> > .../state_tracker/st_glsl_to_tgsi_array_merge.h | 116
> > +++++++++
> > 4 files changed, 403 insertions(+)
> > create mode 100644
> > src/mesa/state_tracker/st_glsl_to_tgsi_array_merge.cpp
> > create mode 100644
> > src/mesa/state_tracker/st_glsl_to_tgsi_array_merge.h
> >
> > diff --git a/src/mesa/Makefile.sources b/src/mesa/Makefile.sources
> > index f910b41d0a..391c62640a 100644
> > --- a/src/mesa/Makefile.sources
> > +++ b/src/mesa/Makefile.sources
> > @@ -519,6 +519,8 @@ STATETRACKER_FILES = \
> > state_tracker/st_glsl_to_nir.cpp \
> > state_tracker/st_glsl_to_tgsi.cpp \
> > state_tracker/st_glsl_to_tgsi.h \
> > + state_tracker/st_glsl_to_tgsi_array_merge.cpp \
> > + state_tracker/st_glsl_to_tgsi_array_merge.h \
> > state_tracker/st_glsl_to_tgsi_private.cpp \
> > state_tracker/st_glsl_to_tgsi_private.h \
> > state_tracker/st_glsl_to_tgsi_temprename.cpp \
> > diff --git a/src/mesa/meson.build b/src/mesa/meson.build
> > index 72d87178aa..df4d727454 100644
> > --- a/src/mesa/meson.build
> > +++ b/src/mesa/meson.build
> > @@ -564,6 +564,8 @@ files_libmesa_gallium = files(
> > 'state_tracker/st_glsl_to_nir.cpp',
> > 'state_tracker/st_glsl_to_tgsi.cpp',
> > 'state_tracker/st_glsl_to_tgsi.h',
> > + 'state_tracker/st_glsl_to_tgsi_array_merge.cpp',
> > + 'state_tracker/st_glsl_to_tgsi_array_merge.h',
> > 'state_tracker/st_glsl_to_tgsi_private.cpp',
> > 'state_tracker/st_glsl_to_tgsi_private.h',
> > 'state_tracker/st_glsl_to_tgsi_temprename.cpp',
> > diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi_array_merge.cpp
> > b/src/mesa/state_tracker/st_glsl_to_tgsi_array_merge.cpp
> > new file mode 100644
> > index 0000000000..52d0d81796
> > --- /dev/null
> > +++ b/src/mesa/state_tracker/st_glsl_to_tgsi_array_merge.cpp
> > @@ -0,0 +1,283 @@
> > +/*
> > + * Copyright © 2017 Gert Wollny
> > + *
> > + * Permission is hereby granted, free of charge, to any person
> > obtaining a
> > + * copy of this software and associated documentation files (the
> > "Software"),
> > + * to deal in the Software without restriction, including without
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute,
> > sublicense,
> > + * and/or sell copies of the Software, and to permit persons to
> > whom the
> > + * Software is furnished to do so, subject to the following
> > conditions:
> > + *
> > + * The above copyright notice and this permission notice
> > (including the next
> > + * paragraph) shall be included in all copies or substantial
> > portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
> > EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> > DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > OTHER
> > + * DEALINGS IN THE SOFTWARE.
> > + */
> > +
> > +#include "program/prog_instruction.h"
> > +#include "util/u_math.h"
> > +#include <ostream>
> > +#include <cassert>
> > +#include <algorithm>
> > +
> > +#include <iostream>
> > +
> > +#include "st_glsl_to_tgsi_array_merge.h"
> > +
> > +#if __cplusplus >= 201402L
> > +#include <memory>
> > +using std::unique_ptr;
> > +using std::make_unique;
> > +#endif
> > +
> > +namespace tgsi_array_merge {
> > +
> > +array_remapping::array_remapping():
> > + target_id(0),
> > + reswizzle(false),
> > + finalized(true)
> > +{
> > +}
> > +
> > +array_remapping::array_remapping(int trgt_array_id, unsigned
> > src_access_mask):
> > + target_id(trgt_array_id),
> > + original_src_access_mask(src_access_mask),
> > + reswizzle(false),
> > + finalized(false)
> > +{
> > +}
> > +
> > +array_remapping::array_remapping(int trgt_array_id, int
> > trgt_access_mask,
> > + int src_access_mask):
> > + target_id(trgt_array_id),
> > + summary_access_mask(trgt_access_mask),
> > + original_src_access_mask(src_access_mask),
> > + reswizzle(true),
> > + finalized(false)
> > +{
> > + for (int i = 0; i < 4; ++i) {
> > + read_swizzle_map[i] = -1;
> > + writemask_map[i] = 0;
> > + }
> > +
> > + int src_swizzle_bit = 1;
> > + int next_free_swizzle_bit = 1;
> > + int k = 0;
> > + bool skip = true;
> > + unsigned last_src_bit = util_last_bit(src_access_mask);
> > +
> > + for (unsigned i = 0; i < 4; ++i, src_swizzle_bit <<= 1) {
> > +
> > + /* The swizzle mapping fills the unused slots with the last
> > used
> > + * component (think temp[A].xyyy) and maps the write mask
> > accordingly.
> > + * Hence, if (i < last_src_bit) skip is true and mappings
> > are only addeed
> > + * for used the components, but for (i >= last_src_bit) the
> > mapping
> > + * is set for remaining slots.
> > + */
> > + if (skip && !(src_swizzle_bit & src_access_mask))
> > + continue;
> > + skip = (i < last_src_bit);
> > +
> > + /* Find the next free access slot in the target.*/
> > + while ((trgt_access_mask & next_free_swizzle_bit) &&
> > + k < 4) {
> > + next_free_swizzle_bit <<= 1;
> > + ++k;
> > + }
> > + assert(k < 4 &&
> > + "Interleaved array would have more then four
> > components");
> > +
> > + /* Set the mapping for this component. */
> > + read_swizzle_map[i] = k;
> > + writemask_map[i] = next_free_swizzle_bit;
> > + trgt_access_mask |= next_free_swizzle_bit;
> > +
> > + /* Update the joined access mask if we didn't just fill the
> > mapping.*/
> > + if (src_swizzle_bit & src_access_mask)
> > + summary_access_mask |= next_free_swizzle_bit;
> > + }
> > +}
> > +
> > +int array_remapping::map_writemask(int writemask_to_map) const
> > +{
> > + assert(is_valid());
> > + if (!reswizzle)
> > + return writemask_to_map;
> > +
> > + assert(original_src_access_mask & writemask_to_map);
>
> You probably want `original_src_access_mask & writemask_to_map ==
> writemask_to_map` here. Otherwise you're only checking for overlap,
> not
> set inclusion.
>
>
> > + int result = 0;
> > + for (int i = 0; i < 4; ++i) {
> > + if (1 << i & writemask_to_map)
> > + result |= writemask_map[i];
> > + }
> > + return result;
> > +}
> > +
> > +uint16_t array_remapping::move_read_swizzles(uint16_t
> > original_swizzle) const
> > +{
> > + assert(is_valid());
> > + if (!reswizzle)
> > + return original_swizzle;
> > +
> > + /* Since
> > + *
> > + * dst.zw = src.xy in glsl actually is MOV dst.__zw src.__xy
> > + *
> > + * when interleaving the arrays the source swizzles must be
> > moved
> > + * according to the changed dst write mask.
> > + */
> > + uint16_t out_swizzle = 0;
> > + for (int idx = 0; idx < 4; ++idx) {
> > + uint16_t orig_swz = GET_SWZ(original_swizzle, idx);
> > + int new_idx = read_swizzle_map[idx];
> > + if (new_idx >= 0)
> > + out_swizzle |= orig_swz << 3 * new_idx;
> > + }
> > + return out_swizzle;
> > +}
> > +
> > +int array_remapping::map_one_swizzle(int swizzle_to_map) const
> > +{
> > + if (!reswizzle)
> > + return swizzle_to_map;
> > +
> > + assert(read_swizzle_map[swizzle_to_map] >= 0);
> > + return read_swizzle_map[swizzle_to_map];
> > +}
> > +
> > +uint16_t array_remapping::map_swizzles(uint16_t old_swizzle) const
> > +{
> > + if (!reswizzle)
> > + return old_swizzle;
> > +
> > + uint16_t out_swizzle = 0;
> > + for (int idx = 0; idx < 4; ++idx) {
> > + uint16_t swz = map_one_swizzle(GET_SWZ(old_swizzle, idx));
> > + out_swizzle |= swz << 3 * idx;
> > + }
> > + return out_swizzle;
> > +}
> > +
> > +void array_remapping::print(std::ostream& os) const
> > +{
> > + static const char xyzw[] = "xyzw";
> > + if (is_valid()) {
> > + os << "[aid: " << target_id;
> > +
> > + if (reswizzle) {
> > + os << " write-swz: ";
> > + for (int i = 0; i < 4; ++i) {
> > + if (1 << i & original_src_access_mask) {
> > + switch (writemask_map[i]) {
> > + case 1: os << "x"; break;
> > + case 2: os << "y"; break;
> > + case 4: os << "z"; break;
> > + case 8: os << "w"; break;
> > + }
> > + } else {
> > + os << "_";
> > + }
> > + }
> > + os << ", read-swz: ";
> > + for (int i = 0; i < 4; ++i) {
> > + if (1 << i & original_src_access_mask &&
> > read_swizzle_map[i] >= 0)
> > + os << xyzw[read_swizzle_map[i]];
> > + else
> > + os << "_";
> > + }
> > + }
> > + os << "]";
> > + } else {
> > + os << "[unused]";
> > + }
> > +}
> > +
> > +void array_remapping::finalize_mappings(array_remapping *arr_map)
> > +{
> > + assert(is_valid());
> > +
> > + array_remapping& forward_map = arr_map[target_id];
> > +
> > + /* If no valid map is provided than we have a final target
> > array
> > + * at the target_id index, no finalization needed. */
> > + if (!forward_map.is_valid())
> > + return;
> > +
> > + /* This mappoints to another mapped array that may need
> > finalization. */
> > + if (!forward_map.is_finalized())
> > + forward_map.finalize_mappings(arr_map);
> > +
> > + /* Now finalize this mapping by translating the map to
> > represent
> > + * a mapping to a final target array (i.e. one that is not
> > mapped).
> > + * This is only necessary if the target_id array map provides
> > reswizzling.
> > + */
> > + if (forward_map.reswizzle) {
> > +
> > + /* If this mapping doesn't have a reswizzle map build one
> > now.*/
> > + if (!reswizzle) {
> > + for (int i = 0; i < 4; ++i) {
> > + if (1 << i & original_src_access_mask) {
> > + read_swizzle_map[i] = i;
> > + writemask_map[i] = 1 << i;
> > + } else {
> > + read_swizzle_map[i] = -1;
> > + writemask_map[i] = 0;
> > + }
> > + }
> > + reswizzle = true;
> > + }
> > +
> > + /* Propagate the swizzle mapping of the forward map.*/
> > + for (int i = 0; i < 4; ++i) {
> > + if ((1 << i & original_src_access_mask) == 0)
> > + continue;
> > + read_swizzle_map[i] =
> > forward_map.map_one_swizzle(read_swizzle_map[i]);
> > + writemask_map[i] =
> > forward_map.map_writemask(writemask_map[i]);
> > + }
> > +
> > + }
> > +
> > + /* Now we can skip the intermediate mapping.*/
> > + target_id = forward_map.target_id;
> > + finalized = true;
> > +}
> > +
> > +/* Required by the unit tests */
> > +bool operator == (const array_remapping& lhs, const
> > array_remapping& rhs)
> > +{
> > + if (lhs.target_id != rhs.target_id)
> > + return false;
> > +
> > + if (lhs.target_id == 0)
> > + return true;
> > +
> > + if (lhs.reswizzle) {
> > + if (!rhs.reswizzle)
> > + return false;
> > +
> > + if (lhs.original_src_access_mask !=
> > rhs.original_src_access_mask)
> > + return false;
> > +
> > + for (int i = 0; i < 4; ++i) {
> > + if (1 << i & lhs.original_src_access_mask) {
> > + if (lhs.writemask_map[i] != rhs.writemask_map[i])
> > + return false;
> > + if (lhs.read_swizzle_map[i] !=
> > rhs.read_swizzle_map[i])
> > + return false;
> > + }
> > + }
> > + } else {
> > + return !rhs.reswizzle;
> > + }
> > + return true;
> > +}
> > +
> > +/* end namespace tgsi_array_merge */
> > +}
> > diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi_array_merge.h
> > b/src/mesa/state_tracker/st_glsl_to_tgsi_array_merge.h
> > new file mode 100644
> > index 0000000000..c74c854e09
> > --- /dev/null
> > +++ b/src/mesa/state_tracker/st_glsl_to_tgsi_array_merge.h
> > @@ -0,0 +1,116 @@
> > +/*
> > + * Copyright © 2017 Gert Wollny
> > + *
> > + * Permission is hereby granted, free of charge, to any person
> > obtaining a
> > + * copy of this software and associated documentation files (the
> > "Software"),
> > + * to deal in the Software without restriction, including without
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute,
> > sublicense,
> > + * and/or sell copies of the Software, and to permit persons to
> > whom the
> > + * Software is furnished to do so, subject to the following
> > conditions:
> > + *
> > + * The above copyright notice and this permission notice
> > (including the next
> > + * paragraph) shall be included in all copies or substantial
> > portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
> > EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> > DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > OTHER
> > + * DEALINGS IN THE SOFTWARE.
> > + */
> > +
> > +#ifndef MESA_GLSL_TO_TGSI_ARRAY_MERGE_H
> > +#define MESA_GLSL_TO_TGSI_ARRAY_MERGE_H
> > +
> > +#include "st_glsl_to_tgsi_private.h"
> > +#include <iosfwd>
> > +
> > +namespace tgsi_array_merge {
> > +
> > +/* Helper class to merge and interleave arrays.
> > + * The interface is exposed here to make unit tests possible.
> > + */
> > +class array_remapping {
> > +public:
> > +
>
> Please avoid empty lines at the beginning of a scope.
>
>
> > + /** Create an invalid mapping that is used as place-holder for
> > + * arrays that are not mapped at all.
> > + */
> > + array_remapping();
> > +
> > + /** Simple remapping that is done when the lifetimes do not
> > + * overlap.
> > + * @param trgt_array_id ID of the array that the new array will
> > + * be interleaved with
> > + */
> > + array_remapping(int trgt_array_id, unsigned src_access_mask);
> > +
> > + /** Component interleaving of arrays.
> > + * @param target_array_id ID of the array that the new array
> > will
> > + * be interleaved with
> > + * @param trgt_access_mask the component mast of the target
> > array
>
> *mask
>
>
> > + * (the components that are already reserved)
> > + * @param orig_component_mask
> > + */
> > + array_remapping(int trgt_array_id, int trgt_access_mask,
> > + int src_access_mask);
>
> Please make sure the comment and declaration match.
>
> Cheers,
> Nicolai
>
>
> > +
> > + /* Defines a valid remapping */
> > + bool is_valid() const {return target_id > 0;}
> > +
> > + /* Resolve the mapping chain so that this mapping remaps to an
> > + * array that is not remapped.
> > + */
> > + void finalize_mappings(array_remapping *arr_map);
> > +
> > + void set_target_id(int tid) {target_id = tid;}
> > +
> > + /* Translates the write mask to the new, interleaved component
> > + * position
> > + */
> > + int map_writemask(int original_src_access_mask) const;
> > +
> > + /* Translates all read swizzles to the new, interleaved
> > component
> > + * swizzles
> > + */
> > + uint16_t map_swizzles(uint16_t original_swizzle) const;
> > +
> > + /** Move the read swizzles to the positiones that correspond to
> > + * a changed write mask.
> > + */
> > + uint16_t move_read_swizzles(uint16_t original_swizzle) const;
> > +
> > + unsigned target_array_id() const {return target_id;}
> > +
> > + int combined_access_mask() const {return summary_access_mask;}
> > +
> > + void print(std::ostream& os) const;
> > +
> > + bool is_finalized() { return finalized;}
> > +
> > + friend bool operator == (const array_remapping& lhs,
> > + const array_remapping& rhs);
> > +
> > + int map_one_swizzle(int swizzle_to_map) const;
> > +
> > +private:
> > + unsigned target_id;
> > + uint16_t writemask_map[4];
> > + int16_t read_swizzle_map[4];
> > + unsigned summary_access_mask:4;
> > + unsigned original_src_access_mask:4;
> > + int reswizzle:1;
> > + int finalized:1;
> > +};
> > +
> > +inline
> > +std::ostream& operator << (std::ostream& os, const
> > array_remapping& am)
> > +{
> > + am.print(os);
> > + return os;
> > +}
> > +
> > +}
> > +#endif
> >
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
