Re: [PATCH v4 3/5] MdeModulePkg: implement generic PCI I/O driver for non-discoverable devices

[Leif Lindholm <leif.lindholm@linaro.org>]

On Fri, Dec 02, 2016 at 06:42:58PM +0000, Ard Biesheuvel wrote:
> >> > The point is that there is nothing clever here for the compiler to do:
> >> > casting a 64-bit int to a 32-bit int is an operation that discards the
> >> > top 32 bits - but the behaviour is entirely defined.
> >> >
> >> > In fact, all architectures other than IA32/ARM use the Math64.c
> >> > implementation of RShiftU64, which simply does the shift.
> >> > And ARM does it only for GCC. I'm not sure why, I can't imagine a GCC
> >> > version that would do anything more complicated than
> >> > ---
> >> > 00000000 <shift>:
> >> >   0:    e1a000c1    asr    r0, r1, #1
> >> >   4:    e12fff1e    bx    lr
> >> > ---
> >> > (and that's with -march=armv4t onwards)
> >>
> >> That's a 32-bit operand. Shifting a 64-bit value will result in a
> >> call to a compiler intrinsic, which is kind of the point of Riuyu's
> >> remark
> >
> > No, it's not.
> > As stated, that is the output from compiling:
> >
> > unsigned int shift(long long val)
> > {
> >   return (val >> 33);
> > }
> >
> > Unless you are claiming long long is 32-bit on ARM.
> >
> > Although I did mess up and make it signed :)
> > When fixed, the code generation is identical but with an lsr instead.
> >
> > The compiler just operates directly on the top 32-bits (in r1), since
> > the ones in r0 are irrelevant to the result.
> 
> OK, but that does mean it is performing optimization up to some point.
> So right shifts of 64-bit quantities by 32 bits or more will be
> 'optimized' by GCC into something that does not rely on the
> intrinsics. But this is not universally true for all toolchains

And I'm OK with a statement of "we can't do this because version X of
toolchain Y does not support it". But I am much less happy for people
to introduce workarounds in functional code because of an issue that
may no longer be relevant for any toolchains actually in use (or even
supported by BaseTools).

At which point can we know this problem goes away and we can program
in regular C instead, if we decide to obsolete some extremely outdated
toolchains?

> (and variable shifts are probably treated differently as well)

It has no effect on intrinsics being required or not, it simply requires
some more stack shuffling. But yes, that removes all options for
compiler shortcuts.

> > Change the shift to 31 and the compiler doesn't do anything cute, so
> > the code is slightly more obvious:
> > ---
> > 00000000 <shift>:
> >    0:    e1a00fa0       lsr     r0, r0, #31
> >    4:    e1800081       orr     r0, r0, r1, lsl #1
> >    8:    e12fff1e       bx      lr
> > ---
> >
> > But even if there were intrinsics generated, shouldn't we support the
> > intrinsic instead of massaging the C code and hope we can prevent it
> > from being generated.
> 
> This is exactly the issue at hand: EDK2 typically does *not* rely on
> such intrinsics, even if we were forced to add them for ARM because
> GCC cannot be instructed to inhibit such calls.
> 
> Actually, I do not agree with the EDK2 position, i.e., I think there
> is no issue relying on routines that are typically provided by the C
> runtime. But this is MdeModulePkg, so it is not up to us.

Indeed it is not.
But this does not sound like a topic we want per-package rules on.

Regards,

Leif