[Mesa-dev] [PATCH 26/26] nir/dead_variables: Configurably work with any variable mode

[Rob Clark]

On Mon, Apr 11, 2016 at 2:58 AM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> On Sat, Mar 26, 2016 at 3:00 PM, Rob Clark <robdclark at gmail.com> wrote:
>>
>> On Sat, Mar 26, 2016 at 5:43 PM, Jason Ekstrand <jason at jlekstrand.net>
>> wrote:
>> > On Sat, Mar 26, 2016 at 8:22 AM, Rob Clark <robdclark at gmail.com> wrote:
>> >> btw, I do remember for lower_io I wanted the mode to be a bitmask so
>> >> we could more easily deal w/ multiple modes in the same pass, but that
>> >> exceeded # of bits somewhere or other.  I suppose maybe nir_var_all is
>> >> good enough..  offhand I can't think of a scenario where we want to
>> >> lower some but not all..
>> >
>> >
>> > I'd rather do that too.  I don't remember why we didn't before (too lazy
>> > to
>> > search the mailing list).  It shouldn't overflow a uint32_t, there are
>> > only
>> > 9 modes last I checked.  We may add more, but it'll be a while before we
>> > add
>> > 22 more.
>>
>> I dug it up, and the issue is in nir_variable_data:
>>
>>       nir_variable_mode mode:5;
>>
>> ofc we could just give more bits to that and be done...
>
>
> Adding connor to the Cc in case he has an opinion...
>
> I think that specifying multiple modes is something that will happen often
> enough that we should just standardize it.  I see three different
> possibilities here:
>
>  1) nir_lower_io takes a nir_var_all enum value which is -1 and indicates
> "everything"
>  2) nir_lower_indirect_derefs takes a uint32_t mode_mask which is expected
> to be a bunch of (1 << mode) ORd together
>  3) We could just convert each of the mode enum values to be a single bit
> and support ORing them together

I like #3

BR,
-R

> (1) is pretty terrible since it only allows one or all and doesn't allow you
> to specify a non-trivial subset.  (2) allows for enum confusion but is a
> fairly standard pattern in mesa today so maybe not too bad.  (3) requires
> more bits when you only want to specify one enum (meh), requires that we
> validate that only one bit is set on a variable, and will be confused with
> the common pattern of (2) in mesa.  On the other hand (3) prints nicely in
> gdb and looks nice when you write it.
>
> All told, I don't like (1) at all and have a mild preference for (2) over
> (3).  However, I could be convinced of (3) should others have a strong
> inclination towards it.
>
> Regardless of what we choose, I'll update this patch and submit patches to
> fix up nir_lower_io and nir_lower_indirect_derefs to match.
>
> --Jason
>