[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.