| Message ID | 1461077734-4327-2-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | 80e5a33da1fcbe54cbcc178f1880e80e297d5fd4 |
| Headers | show |
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? > 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 <ard.biesheuvel@linaro.org> > --- > 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.) > > // Get the cacheability of the region > Status = gDS->GetMemorySpaceDescriptor (*DeviceAddress, &GcdDescriptor); > -- > 2.5.0 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 9 May 2016 at 18:22, Leif Lindholm <leif.lindholm@linaro.org> 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. >> 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 <ard.biesheuvel@linaro.org> >> --- >> 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. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Mon, May 09, 2016 at 06:42:39PM +0200, Ard Biesheuvel wrote: > On 9 May 2016 at 18:22, Leif Lindholm <leif.lindholm@linaro.org> 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 <ard.biesheuvel@linaro.org> > >> --- > >> 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 <leif.lindholm@linaro.org> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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) { // Get the cacheability of the region Status = gDS->GetMemorySpaceDescriptor (*DeviceAddress, &GcdDescriptor);
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, 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 <ard.biesheuvel@linaro.org> --- ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) -- 2.5.0 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel