Re: [Mesa-dev] [PATCH] radeon: remove unnecessary checks
On Tue, 2016-06-14 at 22:44 +0200, Jakob Sinclair wrote:
> On 2016-06-14 20:39, Jan Vesely wrote:
> > I really disagree here. The conditions check whether swizzle is
> > between
> > X and W (as in, only X,Y,Z,W are allowed). The fact that X maps to
> > 0 is
> > irrelevant. removing the checks impairs readability of the code
> > because
> > the lower bound is now inferred (by being 0) rather than explicit.
> >
> > the same comment applies to your v2.
> >
> > Jan
>
> Thanks for the input. Now when I think about it again this is
> probably a
> bad change.
> Didn't think about the lower bound. So this patch should probably not
> be
> pushed.
It'd still be nice to have them fixed.
those lines produce Wtype-limits ("comparison always false due to
limited range of data types") warnings which are rather useful in other
cases, since type-limits can lead to DCE of useful code with aggressive
optimization.
not sure what the best way to do is, though.
Jan
--
Jan Vesely
signature.asc
Description: This is a digitally signed message part
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] radeon: remove unnecessary checks
On 2016-06-14 20:39, Jan Vesely wrote: I really disagree here. The conditions check whether swizzle is between X and W (as in, only X,Y,Z,W are allowed). The fact that X maps to 0 is irrelevant. removing the checks impairs readability of the code because the lower bound is now inferred (by being 0) rather than explicit. the same comment applies to your v2. Jan Thanks for the input. Now when I think about it again this is probably a bad change. Didn't think about the lower bound. So this patch should probably not be pushed. -- Mvh Jakob Sinclair ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] radeon: remove unnecessary checks
On Tue, 2016-06-14 at 20:19 +0200, Jakob Sinclair wrote: > On 2016-06-13 12:02, Nicolai Hähnle wrote: > > > > Meh. This is the kind of thing where Coverity should perhaps just > > shut > > up :/ > > I do agree with you that Coverity should perhaps shut up about this > kinda thing > but I couldn't see a reason to have these checks in the code. They > really didn't > contribute to my understanding of the code. I really disagree here. The conditions check whether swizzle is between X and W (as in, only X,Y,Z,W are allowed). The fact that X maps to 0 is irrelevant. removing the checks impairs readability of the code because the lower bound is now inferred (by being 0) rather than explicit. the same comment applies to your v2. Jan > Although I may be missing > something > important here. > > > Anyway... > > I think for consistency, you should also remove the '- > > PIPE_SWIZZLE_X' > > here, similar to the first hunk. With that changed, > > Forgot about that one. I agree with this change. > > > Reviewed-by: Nicolai Hähnle > > Thanks! -- Jan Vesely signature.asc Description: This is a digitally signed message part ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] radeon: remove unnecessary checks
On 2016-06-13 12:02, Nicolai Hähnle wrote: Meh. This is the kind of thing where Coverity should perhaps just shut up :/ I do agree with you that Coverity should perhaps shut up about this kinda thing but I couldn't see a reason to have these checks in the code. They really didn't contribute to my understanding of the code. Although I may be missing something important here. Anyway... I think for consistency, you should also remove the '- PIPE_SWIZZLE_X' here, similar to the first hunk. With that changed, Forgot about that one. I agree with this change. Reviewed-by: Nicolai Hähnle Thanks! -- Mvh Jakob Sinclair ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] radeon: remove unnecessary checks
On 13.06.2016 03:19, Jakob Sinclair wrote:
PIPE_SWIZZLE_X is always 0 and desc->swizzle is an unsigned char meaning
that desc->swizzle can never be smaller then PIPE_SWIZZLE_X. Removing
these checks doesn't change the code path at all because they would
always give the same result. Issue discovered by Coverity.
Meh. This is the kind of thing where Coverity should perhaps just shut up :/
Anyway...
CID: 1337954
Signed-off-by: Jakob Sinclair
---
I don't have push access so anyone reviewing this could push it. Thanks!
src/gallium/drivers/radeon/r600_texture.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/gallium/drivers/radeon/r600_texture.c
b/src/gallium/drivers/radeon/r600_texture.c
index a1c314e..aa19fc0 100644
--- a/src/gallium/drivers/radeon/r600_texture.c
+++ b/src/gallium/drivers/radeon/r600_texture.c
@@ -1752,10 +1752,9 @@ static void vi_get_fast_clear_parameters(enum
pipe_format surface_format,
return;
for (i = 0; i < 4; ++i) {
- int index = desc->swizzle[i] - PIPE_SWIZZLE_X;
+ int index = desc->swizzle[i];
- if (desc->swizzle[i] < PIPE_SWIZZLE_X ||
- desc->swizzle[i] > PIPE_SWIZZLE_W)
+ if (desc->swizzle[i] > PIPE_SWIZZLE_W)
continue;
if (util_format_is_pure_sint(surface_format)) {
@@ -1781,7 +1780,6 @@ static void vi_get_fast_clear_parameters(enum pipe_format
surface_format,
for (int i = 0; i < 4; ++i)
if (values[i] != main_value &&
desc->swizzle[i] - PIPE_SWIZZLE_X != extra_channel &&
- desc->swizzle[i] >= PIPE_SWIZZLE_X &&
desc->swizzle[i] <= PIPE_SWIZZLE_W)
I think for consistency, you should also remove the '- PIPE_SWIZZLE_X'
here, similar to the first hunk. With that changed,
Reviewed-by: Nicolai Hähnle
return;
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] radeon: remove unnecessary checks
PIPE_SWIZZLE_X is always 0 and desc->swizzle is an unsigned char meaning
that desc->swizzle can never be smaller then PIPE_SWIZZLE_X. Removing
these checks doesn't change the code path at all because they would
always give the same result. Issue discovered by Coverity.
CID: 1337954
Signed-off-by: Jakob Sinclair
---
I don't have push access so anyone reviewing this could push it. Thanks!
src/gallium/drivers/radeon/r600_texture.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/gallium/drivers/radeon/r600_texture.c
b/src/gallium/drivers/radeon/r600_texture.c
index a1c314e..aa19fc0 100644
--- a/src/gallium/drivers/radeon/r600_texture.c
+++ b/src/gallium/drivers/radeon/r600_texture.c
@@ -1752,10 +1752,9 @@ static void vi_get_fast_clear_parameters(enum
pipe_format surface_format,
return;
for (i = 0; i < 4; ++i) {
- int index = desc->swizzle[i] - PIPE_SWIZZLE_X;
+ int index = desc->swizzle[i];
- if (desc->swizzle[i] < PIPE_SWIZZLE_X ||
- desc->swizzle[i] > PIPE_SWIZZLE_W)
+ if (desc->swizzle[i] > PIPE_SWIZZLE_W)
continue;
if (util_format_is_pure_sint(surface_format)) {
@@ -1781,7 +1780,6 @@ static void vi_get_fast_clear_parameters(enum pipe_format
surface_format,
for (int i = 0; i < 4; ++i)
if (values[i] != main_value &&
desc->swizzle[i] - PIPE_SWIZZLE_X != extra_channel &&
- desc->swizzle[i] >= PIPE_SWIZZLE_X &&
desc->swizzle[i] <= PIPE_SWIZZLE_W)
return;
--
2.8.3
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
