[Mesa-dev] [PATCH 11/21] glsl: Store ir_variable::ir_type in 8 bits instead of 32

[Ian Romanick]

On 05/28/2014 12:44 PM, Ilia Mirkin wrote:
> On Wed, May 28, 2014 at 3:39 PM, Ian Romanick <idr at freedesktop.org> wrote:
>> On 05/28/2014 12:17 PM, Ian Romanick wrote:
>>> On 05/27/2014 08:28 PM, Matt Turner wrote:
>>>> On Tue, May 27, 2014 at 7:49 PM, Ian Romanick <idr at freedesktop.org> wrote:
>>>>> From: Ian Romanick <ian.d.romanick at intel.com>
>>>>>
>>>>> No change in the peak ir_variable memory usage in a trimmed apitrace of
>>>>> dota2 on 64-bit.
>>>>>
>>>>> No change in the peak ir_variable memory usage in a trimmed apitrace of
>>>>> dota2 on 32-bit.
>>>>>
>>>>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>>>>> ---
>>>>>  src/glsl/ir.h | 5 +++--
>>>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/src/glsl/ir.h b/src/glsl/ir.h
>>>>> index 7faee74..bc02f6e 100644
>>>>> --- a/src/glsl/ir.h
>>>>> +++ b/src/glsl/ir.h
>>>>> @@ -92,12 +92,13 @@ enum ir_node_type {
>>>>>   */
>>>>>  class ir_instruction : public exec_node {
>>>>>  private:
>>>>> -   enum ir_node_type ir_type;
>>>>> +   uint8_t ir_type;
>>>>>
>>>>>  public:
>>>>>     inline enum ir_node_type get_ir_type() const
>>>>>     {
>>>>> -      return this->ir_type;
>>>>> +      STATIC_ASSERT(ir_type_max < 256);
>>>>> +      return (enum ir_node_type) this->ir_type;
>>>>>     }
>>>>>
>>>>>     /**
>>>>> --
>>>>> 1.8.1.4
>>>>
>>>> Instead of doing this, you can mark the enum type with the PACKED
>>>> attribute. I did this in a similar change in i965 already. See
>>>> http://lists.freedesktop.org/archives/mesa-dev/2014-February/054643.html
>>>>
>>>> This way we still get enum type checking and warnings out of switch
>>>> statements and such.
>>>
>>> Hmm... that would mean that patch 10 wouldn't strictly be necessary.
>>> The disadvantage is that the next patch would need (right?) some changes
>>> for MSVC, especially on 32-bit.  I think it would need to be
>>>
>>> #if sizeof(ir_node_type) < sizeof(void *)
>>> # define PADDING_BYTES (sizeof(void *) - sizeof(ir_node_type))
>>> #else
>>> # define PADDING_BYTES sizeof(void *)
>>> #  if (__GNUC__ >= 3)
>>> #    error "GCC did us wrong."
>>> #  endif
>>> #endif
>>>
>>>   uint8_t padding[PADDING_BYTES];
>>
>> After Patrick pointed out my obvious lack of C preprocessor skills, I
>> think the only alternative is:
>>
>>    uint8_t padding[sizeof(ir_node_type) < sizeof(void *)
>>                    ? sizeof(void *) - sizeof(ir_node_type)
>>                    : sizeof(void *)];
>>
>> I'd probably hide that with a #define and a big comment, but it's
>> still a bit ugly.
> 
> Just a thought:
> 
> union {
>   enum ir_node_type type;
>   char *padding;
> }
> 
> And start at padding[sizeof(type)]. Or you could do something a little
> uglier like
> 
> union {
>   struct {
>     enum ir_node_type type;
>     char data[0];

I don't think C++ lets you do data[0], but data[1] should also work.

>   }
>   void *padding;
> }
> 
> And then I think that data should point at the right place. BTW, all
> of the above is untested... I could imagine that e.g. struct creates
> funny alignment requirements... but you could pack it.

On 32-bit Windows, sizeof(void*) == sizeof(enum ir_node_type).  In that
case, you can't rely on data having any usable storage.  I believe that
would severly complicate any potential users of this storage... and
seems ripe for build breaks (or worse).

The idea with my crazy code above is that when sizeof(void*) ==
sizeof(enum ir_node_type) you get sizeof(void*) for the padding space.
That wastes some space on 32-bit Windows, but it keeps all the rest of
the code simple.

>   -ilia