Skip to content

Conversation

@taoliq
Copy link
Contributor

@taoliq taoliq commented Nov 18, 2025

According #1450 , refactor some code to avoid branches

@lemire
Copy link
Member

lemire commented Nov 18, 2025

It is reasonable.

@lemire lemire merged commit 667d0ed into simdjson:master Nov 18, 2025
83 checks passed
@lemire
Copy link
Member

lemire commented Nov 18, 2025

Merged.

}
bool isNegative = e < 0;
e = isNegative ? -e : e;
*buf++ = isNegative ? '-' : '+';

Choose a reason for hiding this comment

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

Replacing if statements with ternary operators doesn't make the code branchless. It's just different syntax.

Copy link
Member

Choose a reason for hiding this comment

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

Correct, although the new code is more elegant I believe. No?

The PR removed a branch later on by always printing two digits (a common convention). So this removes a branch in some cases and might be faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The branchless here means no jmp or other branch instruction in assembly code, using comv instead.
So the ternary operators is just a easy way for compiler to generate comv, the clang generated assembly code is

        mov     ecx, esi
        neg     ecx
        cmovs   ecx, esi
        shr     esi, 31
        add     sil, sil
        add     sil, 43
        mov     byte ptr [rdi], sil

But I have to say, even with or without ternary operator, the compiler still generate the same assembly code, see here

Copy link
Member

Choose a reason for hiding this comment

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

@taoliq Yes. But the reason for it is due to how the code is written. In both branches in the old code, we do much the same work... so that the compiler can recognize the pattern and optimize accordingly.

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.

3 participants