On Mon, May 09, 2016 at 06:42:39PM +0200, Ard Biesheuvel wrote: > On 9 May 2016 at 18:22, Leif Lindholm <[email protected]> wrote: > > On Tue, Apr 19, 2016 at 04:55:31PM +0200, Ard Biesheuvel wrote: > >> We manage to use both an AND operation with 'gCacheAlignment - 1' and a > >> modulo operation with 'gCacheAlignment' in the same compound if statement. > >> Since gCacheAlignment is a global of which the compiler cannot guarantee > >> that it is a power of two, > > > > Are you saying this is an "undefined behaviour" disaster waiting to > > happen? > > No, not at all. It is just unnecessary to invoke the division routines.
OK, still worthwhile. > >> simply use the AND version, and use it against > >> the bitwise OR of the two values to be tested. > >> > >> Contributed-under: TianoCore Contribution Agreement 1.0 > >> Signed-off-by: Ard Biesheuvel <[email protected]> > >> --- > >> ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 3 +-- > >> 1 file changed, 1 insertion(+), 2 deletions(-) > >> > >> diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c > >> b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c > >> index 1e6b288b10b9..834afdba10ef 100644 > >> --- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c > >> +++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c > >> @@ -92,8 +92,7 @@ DmaMap ( > >> > >> *Mapping = Map; > >> > >> - if ((((UINTN)HostAddress & (gCacheAlignment - 1)) != 0) || > >> - ((*NumberOfBytes % gCacheAlignment) != 0)) { > >> + if ((((UINTN)HostAddress | *NumberOfBytes) & (gCacheAlignment - 1)) != > >> 0) { > > > > While the above is correct code, and less verbose, I am a bear of very > > little brain, and OR:ing an address with a counter makes it hurt. > > > > Could the above instead be (more verbose but hopefully not relevantly > > less efficient with a halfway decent compiler): > > if ((((UINTN)HostAddress & (gCacheAlignment - 1)) != 0) || > > (*NumberOfBytes & (gCacheAlignment - 1) != 0)) > > > > (It would have the benefit of making the diffstat immediately obvious.) > > Yeah, that is also fine. I just noticed the mask and the modulo > operation involving the same variable. I agree your variant is more elegant, but it messes with my zen :) Reviewed-by: Leif Lindholm <[email protected]> _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
