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

[Ilia Mirkin]

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];
  }
  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.

  -ilia