Mesh: Make edge_other_vert function branchless #114682

Iliya Katushenock · 2023-11-09T19:38:00+01:00

Iliya Katushenock commented

2023-11-09 19:38:00 +01:00

This function shouldn't return invalid index. Instead assertion in used.

Iliya Katushenock added 1 commit 2023-11-09 19:38:11 +01:00

1a739ddc27 init

Iliya Katushenock requested review from Hans Goudey 2023-11-09 19:38:18 +01:00

Hans Goudey commented

2023-11-09 20:40:30 +01:00

I think this has limitations for large vertex indices. If it's not actually faster, might be better to stick with the existing code. In other words, the PR needs to be a bit more convincing I guess.

Iliya Katushenock commented

2023-11-10 13:13:15 +01:00

I did short research.
There is kind similar thing called swapping by using sum (Stack Overflow with explanation why that is not good approach: Stack Overflow: Answer to the question).
Lookup for another vertex of edge have the same bad side.
But that still works for me on godbolt.
I pretty sure that is not safe approath and this shouldn't used at all (even in already exist code).

But, current case have some differences, that can protect us:

Edge vertex indices will never be the same.
Edge vertex indices will never be negative.

This means that the order of operations matters.

tmp = (edge[0] - vert) will never be undefined in case of limitation from above.
tmp + edge[1] the same case, if expected index is not an overflow then result will be correct too.

Fixed version

About the code with regular branches:

Assembly code of functions without optimizations from godbolt (GCC x86-64) on godbolt

edge_other_vert(std::pair<int, int>, int): // branchless
        push    rbp
        mov     rbp, rsp
        mov     QWORD PTR [rbp-8], rdi
        mov     DWORD PTR [rbp-12], esi
        mov     eax, DWORD PTR [rbp-8]
        sub     eax, DWORD PTR [rbp-12]
        mov     edx, eax
        mov     eax, DWORD PTR [rbp-4]
        add     eax, edx
        pop     rbp
        ret
edge_other_vert_2(std::pair<int, int>, int): // branchfull
        push    rbp
        mov     rbp, rsp
        mov     QWORD PTR [rbp-8], rdi
        mov     DWORD PTR [rbp-12], esi
        mov     eax, DWORD PTR [rbp-8]
        cmp     DWORD PTR [rbp-12], eax
        jne     .L4
        mov     eax, DWORD PTR [rbp-4]
        jmp     .L5
.L4:
        mov     eax, DWORD PTR [rbp-4]
        cmp     DWORD PTR [rbp-12], eax
        jne     .L6
        mov     eax, DWORD PTR [rbp-8]
        jmp     .L5
.L6:

Version with branches is larger and contains at least one comparison.
Potential using of this function in hot loops and simd code could be less productive than it can be.

That means branchless function make sense in future respective at least.

That goal of that pull request is not an optimization. But just to measure impact of that change i did benchmark, but there is no any real change. I hope this will be occur in simd tests...

I did short research. There is kind similar thing called swapping by using sum (Stack Overflow with explanation why that is not good approach: [Stack Overflow: Answer to the question](https://stackoverflow.com/questions/30419394/why-does-this-swap-using-simple-addition-and-subtraction-wont-overflow/30419507#30419507)). Lookup for another vertex of edge have the same bad side. But that still works for me on [godbolt](https://godbolt.org/z/zcnqjx5GM). I pretty sure that is not safe approath and this shouldn't used at all (even in already exist code). But, current case have some differences, that can protect us: 1. Edge vertex indices will never be the same. 2. Edge vertex indices will never be negative. This means that the order of operations matters. 1. `tmp = (edge[0] - vert)` will never be undefined in case of limitation from above. 2. `tmp + edge[1]` the same case, if expected index is not an overflow then result will be correct too. [Fixed version](https://godbolt.org/z/Wcn8zod4W) --- About the code with regular branches: Assembly code of functions without optimizations from godbolt (GCC x86-64) [on godbolt](https://godbolt.org/z/q57bTrvo7) ```Asm edge_other_vert(std::pair<int, int>, int): // branchless push rbp mov rbp, rsp mov QWORD PTR [rbp-8], rdi mov DWORD PTR [rbp-12], esi mov eax, DWORD PTR [rbp-8] sub eax, DWORD PTR [rbp-12] mov edx, eax mov eax, DWORD PTR [rbp-4] add eax, edx pop rbp ret edge_other_vert_2(std::pair<int, int>, int): // branchfull push rbp mov rbp, rsp mov QWORD PTR [rbp-8], rdi mov DWORD PTR [rbp-12], esi mov eax, DWORD PTR [rbp-8] cmp DWORD PTR [rbp-12], eax jne .L4 mov eax, DWORD PTR [rbp-4] jmp .L5 .L4: mov eax, DWORD PTR [rbp-4] cmp DWORD PTR [rbp-12], eax jne .L6 mov eax, DWORD PTR [rbp-8] jmp .L5 .L6: ``` Version with branches is larger and contains at least one comparison. Potential using of this function in *hot loops* and simd code could be less productive than it can be. That means branchless function make sense in future respective at least. That goal of that pull request is not an optimization. But just to measure impact of that change i did benchmark, but there is no any real change. I hope this will be occur in simd tests...

Hans Goudey reviewed 2023-11-12 22:07:58 +01:00

source/blender/blenkernel/BKE_mesh.hh

						
				@ -280,2 +274,2 @@

				  }

				  return -1;

				  BLI_assert(ELEM(vert, edge[0], edge[1]));

				  BLI_assert(vert >= 0);

Hans Goudey commented

2023-11-12 22:07:17 +01:00

This assert seems to be redundant with the others.

mod_moder marked this conversation as resolved

source/blender/blenkernel/BKE_mesh.hh Outdated

						
				@ -282,0 +275,4 @@

				  BLI_assert(vert >= 0);

				  BLI_assert(edge[0] >= 0);

				  BLI_assert(edge[1] >= 0);

				  return edge[0] + edge[1] - vert;