[Mesa-dev] [PATCH v3 11/18] mesa/macros: add power-of-two assertions for alignment macros

[Anuj Phogat]

On Thu, Jun 25, 2015 at 3:22 PM, Nanley Chery <nanleychery at gmail.com> wrote:
> How about if I create a patch which puts the greater than 0 check into
> is_power_of_two()?
>
> (value > 0) && (value & (value - 1)) == 0);
>
Not a bad idea except that you'll need to fix conditions at all other places
which expect the current behavior. I'll leave it up to you. FYI gallium also
have a helper function util_is_power_of_two() which returns true for 0.

> Thanks,
> Nanley
>
> On Wed, Jun 24, 2015 at 3:22 PM, Anuj Phogat <anuj.phogat at gmail.com> wrote:
>> On Mon, Jun 22, 2015 at 4:02 PM, Nanley Chery <nanleychery at gmail.com> wrote:
>>> From: Nanley Chery <nanley.g.chery at intel.com>
>>>
>>> ALIGN and ROUND_DOWN_TO both require that the alignment value passed
>>> into the macro be a power of two in the comments. Using software assertions
>>> verifies this to be the case.
>>>
>>> v2: use static inline functions instead of gcc-specific statement expressions.
>>>
>>> Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
>>> ---
>>>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp |  2 +-
>>>  src/mesa/main/macros.h                   | 16 +++++++++++++---
>>>  2 files changed, 14 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>>> index 59081ea..1a57784 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>>> @@ -134,7 +134,7 @@ fs_visitor::nir_setup_outputs(nir_shader *shader)
>>>                                 : var->type->vector_elements;
>>>
>>>        if (stage == MESA_SHADER_VERTEX) {
>>> -         for (int i = 0; i < ALIGN(type_size(var->type), 4) / 4; i++) {
>>> +         for (unsigned int i = 0; i < ALIGN(type_size(var->type), 4) / 4; i++) {
>>>              int output = var->data.location + i;
>>>              this->outputs[output] = offset(reg, 4 * i);
>>>              this->output_components[output] = vector_elements;
>>> diff --git a/src/mesa/main/macros.h b/src/mesa/main/macros.h
>>> index 0608650..4a640ad 100644
>>> --- a/src/mesa/main/macros.h
>>> +++ b/src/mesa/main/macros.h
>>> @@ -684,7 +684,7 @@ minify(unsigned value, unsigned levels)
>>>   * Note that this considers 0 a power of two.
>>>   */
>>>  static inline bool
>>> -is_power_of_two(unsigned value)
>>> +is_power_of_two(uintptr_t value)
>>>  {
>>>     return (value & (value - 1)) == 0;
>>>  }
>>> @@ -700,7 +700,12 @@ is_power_of_two(unsigned value)
>>>   *
>>>   * \sa ROUND_DOWN_TO()
>>>   */
>>> -#define ALIGN(value, alignment)  (((value) + (alignment) - 1) & ~((alignment) - 1))
>>> +static inline uintptr_t
>>> +ALIGN(uintptr_t value, uintptr_t alignment)
>>> +{
>>> +      assert(is_power_of_two(alignment));
>> Also handle the 0 alignment. is_power_of_two() returns true for 0.
>> Use assert(alignment > 0 && is_power_of_two(alignment))?
>>> +      return (((value) + (alignment) - 1) & ~((alignment) - 1));
>>> +}
>>>
>>>  /**
>>>   * Align a value down to an alignment value
>>> @@ -713,7 +718,12 @@ is_power_of_two(unsigned value)
>>>   *
>>>   * \sa ALIGN()
>>>   */
>>> -#define ROUND_DOWN_TO(value, alignment) ((value) & ~(alignment - 1))
>>> +static inline uintptr_t
>>> +ROUND_DOWN_TO(uintptr_t value, uintptr_t alignment)
>>> +{
>>> +      assert(is_power_of_two(alignment));
>> Here as well.
>>> +      return ((value) & ~(alignment - 1));
>>> +}
>>>
>>>
>>>  /** Cross product of two 3-element vectors */
>>> --
>>> 2.4.2
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>> With above changes and indentation fixes:
>> Reviewed-by: Anuj Phogat <anuj.phogat at gmail.com>