Skip to content

Conversation

startewho
Copy link
Contributor

@startewho startewho commented Jun 15, 2024

when i want to build the x86 quickjs and found the _BitScanReverse used in the ctuils.h
and i found the method like this :
tqfx/liba@005abe2
maybe it 's not correctly .
and in my computer ,it build success.
image
seems also fix #430

@startewho startewho mentioned this pull request Jun 15, 2024
@saghul
Copy link
Contributor

saghul commented Jun 15, 2024

Thanks! Any chance you can add a CI target that tests Windows x86, so the build is actually tested?

cutils.h Outdated
Comment on lines 209 to 216
#elif(_MSC_VER)
unsigned long i = 0, hi = 0;
if (_BitScanReverse(&hi, a>> 32))
{
return (int)hi + 32;
}
_BitScanReverse(&i, (uint32_t)a);
return (int)i;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow the local coding style:

  • use `#elif defined(_MSC_VER)
  • indent the unsigned long i = 0, hi = 0; definition
  • add the missing space in a >> 32
  • use the K&R style for if (_BitScanReverse(&hi, a >> 32)) {
  • the return value seems inconsistent with that of _BitScanReverse64.

are __lzcnt, __lzcnt32 or __lzcnt64 available this Windows target platform? it would allow for a simpler implementation.

Comment on lines 93 to 108
/* WARNING: undefined if a = 0 */
/* WARNING: undefined if a = 0 */
static inline int clz64(uint64_t a)
{
#if defined(_MSC_VER) && !defined(__clang__)
#if defined(_MSC_VER)&& defined(_WIN64) && !defined(__clang__)
unsigned long index;
_BitScanReverse64(&index, a);
return 63 - index;
#elif(_MSC_VER)
unsigned long i = 0, hi = 0;
if (_BitScanReverse(&hi, a>> 32))
{
return (int)hi + 32;
}
_BitScanReverse(&i, (uint32_t)a);
return (int)i;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same problems as above and duplicate comment line.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If __lzcnt, __lzcnt32 or __lzcnt64 are available all current Windows targets, using these intrinsics would allow for a simpler implementation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@startewho: Why not use the version quoted by @mekhontsev in the issue #430:

static inline int clz64(uint64_t a)
{
#if defined(_MSC_VER) && !defined(__clang__)
#if INTPTR_MAX == INT64_MAX
    unsigned long index;
    _BitScanReverse64(&index, a);
    return 63 - index;
#else
    if (a >> 32)
        return clz32((unsigned)(a >> 32));
    else
        return clz32((unsigned)a) + 32;
#endif	
#else
    return __builtin_clzll(a);
#endif
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If __lzcnt, __lzcnt32 or __lzcnt64 are available all current Windows targets, using these intrinsics would allow for a simpler implementation.

https://learn.microsoft.com/zh-cn/cpp/intrinsics/lzcnt16-lzcnt-lzcnt64?view=msvc-170
it seems only support amd and intel cpu.

@chqrlie
Copy link
Collaborator

chqrlie commented Jun 16, 2024

@startewho can you share your compiler configuration so @saghul can update the CI with your target?

@startewho
Copy link
Contributor Author

@startewho can you share your compiler configuration so @saghul can update the CI with your target?

I just use the vs2022 open the quickjs file folder,and copy the x64-debug to x86
here is my setting

CMakeSettings.json

image

{
  "configurations": [
    {
      "name": "x64-Debug",
      "generator": "Ninja",
      "configurationType": "Debug",
      "inheritEnvironments": [ "msvc_x64_x64" ],
      "buildRoot": "${projectDir}\\out\\build\\${name}",
      "installRoot": "${projectDir}\\out\\install\\${name}",
      "cmakeCommandArgs": "",
      "buildCommandArgs": "",
      "ctestCommandArgs": ""
    },
    {
      "name": "x86-Debug",
      "generator": "Ninja",
      "configurationType": "Debug",
      "buildRoot": "${projectDir}\\out\\build\\${name}",
      "installRoot": "${projectDir}\\out\\install\\${name}",
      "cmakeCommandArgs": "",
      "buildCommandArgs": "",
      "ctestCommandArgs": "",
      "inheritEnvironments": [ "msvc_x86" ],
      "variables": []
    }
  ]
}

@saghul
Copy link
Contributor

saghul commented Jun 17, 2024

Your branch is protected, so I pushed the PR (retained your authorship) + CI here: #436

@saghul
Copy link
Contributor

saghul commented Jun 17, 2024

Fixed in #436, cheers!

@saghul saghul closed this Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MSVC clz64 for x86

3 participants