Re: [Minios-devel] [PATCH 09/40] arm64: add the basic helpers for arm64

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Shijie,

On 06/11/17 10:02, Huang Shijie wrote:
> On Fri, Nov 03, 2017 at 04:58:52PM +0000, Julien Grall wrote:
> > Hi Shijie,
> >
> > On 03/11/17 03:11, Huang Shijie wrote:
> > > This patch adds the basic helpers in header os.h for arm32 and arm64:
> > This is slightly untrue. arm32 helpers already exists. You only move them in
> > separate headers. But they should really be moved in a separate patch.
> okay, I can change the comment. I do not want to add another patch for
> the moving.
Why? A patch should only do one logical thing. I.e either code movement 
or adding new code.
This is making easier for the reviewers to understand your patch.

[...]

> > >    {
> > > -    int clz;
> > > -
> > > -    /* xxxxx10000 = word
> > > -     * xxxxx01111 = word - 1
> > > -     * 0000011111 = word ^ (word - 1)
> > > -     *      4     = 31 - clz(word ^ (word - 1))
> > > -     */
> > > -
> > > -    __asm__ (
> > > -        "sub r0, %[word], #1\n"
> > > -        "eor r0, r0, %[word]\n"
> > > -        "clz %[clz], r0\n":
> > > -        /* Outputs: */
> > > -        [clz] "=r"(clz):
> > > -        /* Inputs: */
> > > -        [word] "r"(word):
> > > -        /* Clobbers: */
> > > -        "r0");
> > > -
> > > -    return 31 - clz;
> > > +       return __builtin_ctzl(word);
> > Are you sure __builtin_ctzl is defined everywhere? Furthermore, this does
> Does the arm32-gcc not provide the __builtin_ctzl()?
Well, I am asking you that question. So why are you asking me back?

You are the person you submit this patch, so you likely now why you used 
it and check whether all the compilers (e.g GCC and Clang) supports it 
and from which version?
Cheers,

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/cgi-bin/mailman/listinfo/minios-devel