> >>
> >> From: Ming Qian
> >>> Sent: 09 March 2022 05:02
> >> ...
> >>> 3. use 'val >> 1' instead of ' val / 2'
> >>
> >> The compiler should do that anyway.
> >>
> >> Especially for unsigned values.
> >> And it has the wrong (different) rounding for negative values.
> >>
> >> David
> >>
> >
> > Hi David,
> > Yes, you are right, if the value is negative, the behavior is wrong.
> > But here, the value type is u32, so I think it's OK.
>
> Well, it depends on the semantic intent, really. If you're packing a bitfield
> which encodes bits 31:1 of some value then a shift is the most appropriate
> operation. However if you're literally calculating half of a value for, say, a 50%
> threshold level, or the number of 16-bit words represented by a byte length,
> then semantically it's a division, so it should use the divide operator rather
> than obfuscating it behind a shift. Constant division is something that even
> the most basic optimising compiler should handle with ease.
>
Hi Robin,
Thanks for the detailed explanation, and I agree with you, I will use " / 2" in the v2 patch as it's indeed calculating half of a value.
> One more thing that's not the fault of this patch, but stood out in the
> context:
>
> @@ -1566,7 +1568,7 @@ static bool vpu_malone_check_ready(struct
> vpu_shared_addr *shared, u32 instance)
> u32 wptr = desc->wptr;
> u32 used = (wptr + size - rptr) % size;
>
> - if (!size || used < size / 2)
> + if (!size || used < (size >> 1))
> return true;
>
> return false;
>
> That's not safe: if "size" is 0 then the undefined behaviour has already
> happened before the "!size" check is reached. If "size" really can be 0, then it
> needs to be checked *before* it is used as a divisor to calculate "used".
>
> Robin.
Yes, it's problem, and Dan has also pointed it, I 'll fix it in another patch.
Ming
