I stumble upon a C++ function that will be a really illustrative example of how refactoring can both shorten and optimize code. Today, we'll need to dust off our human brains in the era of vibe coding and recall what it's like to know what the good is and what the bad is—after all, someone has to do it.
The code snippet below from the vibe-coded project VibeTensor. I like exploring such projects as if I were a naturalist studying a new ecosystem—you can meet with my "field reports" here and there. So, I'm very curious to discover the genesis of new defects and flaws in code. Why? It helps us better understand and describe the challenges code can face in the GenAI era.
One of my discoveries is code bloat that hurts readability and compilers' optimization. The following example showcases it in all its beauty.
static TensorImpl make_contiguous_tensor(const std::vector<int64_t>& sizes) {
const std::size_t nd = sizes.size();
std::vector<int64_t> strides(nd, 0);
int64_t acc = 1;
for (std::ptrdiff_t i = static_cast<std::ptrdiff_t>(nd) - 1; i >= 0; --i) {
strides[static_cast<std::size_t>(i)] = acc;
const auto sz = sizes[static_cast<std::size_t>(i)];
acc *= (sz == 0 ? 1 : sz);
}
int64_t ne = 1;
bool any_zero = false;
for (auto s : sizes) {
if (s == 0) {
any_zero = true;
break;
}
ne *= s;
}
if (any_zero) {
ne = 0;
}
....
}
It may seem that code size and performance are not so critical because they're used in tests.
However, the code is duplicated across nine files, and ideally, that shouldn't happen at all, but if code is going to spread like this, it should at least stay compact.
Also, we shouldn't forget that the generated C++ code must be optimized because the C++'s main purpose is to create highly efficient apps. In this case, we have to deal only with the slow test, but in another context, such patterns would significantly downgrade app performance and the negative impact would only grow as more flawed generated code accumulates over time.
Slow C++ code goes against the idea of this programming language. That's why it's still a must-have to enhance the expertise in determining whether the code is well-written and how it can be refined, shortened, or optimized. Advanced knowledge can also help in regenerating code with better prompts.
So, let's practice and do some refactoring. Let's start with this code fragment:
int64_t ne = 1;
bool any_zero = false;
for (auto s : sizes) {
if (s == 0) {
any_zero = true;
break;
}
ne *= s;
}
if (any_zero) {
ne = 0;
}
All the array elements are multiplied. If it encounters 0, the loop terminates to avoid unnecessarily processing the remaining array elements because it'll still set the result to zero.
The any_zero flag resets the variable that stores the product, but we can go easier:
int64_t ne = 1;
for (auto s : sizes) {
if (s == 0) {
ne = 0;
break;
}
ne *= s;
}
When we encounter 0, ne will be set to 0 and the loop will end. We have another place for simplification: modern processors perform multiplication operations quickly, so we can do the multiplication first and then check it.
int64_t ne = 1;
for (auto s : sizes) {
ne *= s;
if (s == 0) {
break;
}
}
The code's functionality hasn't changed, but it's become shorter and, in my opinion, even easier to understand.
If we already know that the array is small or that performance isn't critical (for example, because the function is called infrequently), we could simplify it even further:
int64_t ne = 1;
for (auto s : sizes) {
ne *= s;
}
Let's not think about that, since further actions make the loop be absorbed by another one.
Now let's go back to the beginning of the function.
static TensorImpl make_contiguous_tensor(const std::vector<int64_t>& sizes) {
const std::size_t nd = sizes.size();
std::vector<int64_t> strides(nd, 0);
int64_t acc = 1;
for (std::ptrdiff_t i = static_cast<std::ptrdiff_t>(nd) - 1; i >= 0; --i) {
strides[static_cast<std::size_t>(i)] = acc;
const auto sz = sizes[static_cast<std::size_t>(i)];
acc *= (sz == 0 ? 1 : sz);
}
The static_cast makes code clunky. The first type casting is needed so that certain compilers or static analyzers don't flag odd arithmetic operations.
std::ptrdiff_t i = static_cast<std::ptrdiff_t>(nd) - 1;
Let's consider what would happen if we removed the static_cast and the input array turned out to be empty:
- If the array is empty, then
nd = 0; - If we subtract one from an unsigned zero, we get
SIZE_MAX, that is, a very large positive unsigned number. - The
SIZE_MAXof thesize_ttype is implicitly converted toptrdiff_tand stored in theivariable. It turns out thati = -1works as intended. But this is exactly where warnings may be issued. We initializeptrdiff_twith a number that is larger than the range of the largest number it can hold. Before C++20, the result was implementation-defined. Since C++20, the behavior is well-defined.
As a result, it'd be better to leave the first static_cast as is. We can't say the same about the rest. They make no sense at all.
In the vector class, the [] operator takes an argument of the size_type type (typically an alias of size_t). The i variable value will automatically be converted to size_t, and there's nothing unsafe or suspicious about it. The explicit casts only add visual noise, so they're better removed.
for (std::ptrdiff_t i = static_cast<std::ptrdiff_t>(nd) - 1; i >= 0; --i) {
strides[i] = acc;
const auto sz = sizes[i];
acc *= (sz == 0 ? 1 : sz);
}
You've taken the first step. But can we simplify this line even further? Let's see—I'd like to write something like this:
for (auto sz : std::ranges::views::reverse(sizes)) {
strides[???? i ????] = acc;
acc *= (sz == 0 ? 1 : sz);
}
You still need the i variable to iterate through the strides array from the end, so we can't eliminate i completely. But we can do something like that:
int64_t acc = 1;
size_t i = nd;
for (auto sz : std::ranges::views::reverse(sizes)) {
strides[--i] = acc;
acc *= (sz == 0 ? 1 : sz);
}
To be honest, I don't like that --i has made the code more complicated and forces us to pause and realize why the variable is decremented in the beginning and then is used to access an array element.
Okay, there is less code now, and it's easier to scan through it quickly, so half the work is done. But the code is still ultimately hard to read—it's neither clearer nor intrinsically harder.
Let's take a look at the result.
static TensorImpl make_contiguous_tensor(const std::vector<int64_t>& sizes) {
const std::size_t nd = sizes.size();
std::vector<int64_t> strides(nd, 0);
int64_t acc = 1;
size_t i = nd;
for (auto sz : std::ranges::views::reverse(sizes)) {
strides[--i] = acc;
acc *= (sz == 0 ? 1 : sz);
}
int64_t ne = 1;
for (auto s : sizes) {
ne *= s;
if (s == 0) {
break;
}
}
....
}
Now that there is less code, it becomes clear that the second loop is redundant. In the first loop, we iterate through all the elements. So why not multiply them right away?
static TensorImpl make_contiguous_tensor(const std::vector<int64_t>& sizes) {
const std::size_t nd = sizes.size();
std::vector<int64_t> strides(nd, 0);
int64_t acc = 1;
size_t i = nd;
int64_t ne = 1;
for (auto sz : std::ranges::views::reverse(sizes)) {
strides[--i] = acc;
acc *= (sz == 0 ? 1 : sz);
ne *= sz;
}
....
}
Oh, it's an absolute beauty. It's not a big deal that we multiply all the elements together, even though one of them might be zero. Microprocessors are multiplying rapidly these days. You could lose more by re-accessing all the elements in the second loop.
What else is left? We don't need to initialize the strides container with zeros. All of its elements will be overwritten anyway.
std::vector<int64_t> strides(nd, 0); // delete the second argument
I think that's all. We can also make one more small change: remove the nd variable. It's not needed here or in the code that follows.
As a result, we have:
static TensorImpl make_contiguous_tensor(const std::vector<int64_t>& sizes) {
auto q = sizes.size();
std::vector<int64_t> strides(q);
int64_t acc = 1;
int64_t ne = 1;
for (auto sz : std::ranges::views::reverse(sizes)) {
strides[--q] = acc;
acc *= (sz == 0 ? 1 : sz);
ne *= sz;
}
....
}
Hooray! We cut the code in half: from 20 lines down to just 9!
What's inside the assembly code?
The function is shorter now, but does that also improve performance? Yes! Let's take a look at the main section of the assembly code for the original function version.
Clang is used with the -O2 option:
.LBB1_7:
mov rax, rsi
mov qword ptr [rbx + 8*r12 - 16], rsi
mov rsi, qword ptr [r13 + 8*r12 - 16]
cmp rsi, 1
adc rsi, 0
imul rsi, rax
dec r12
cmp r12, 1
ja .LBB1_7
mov r12d, 1
cmp rbp, r13
je .LBB1_9
.LBB1_4:
mov rax, qword ptr [r13]
test rax, rax
je .LBB1_5
imul r12, rax
add r13, 8
cmp r13, rbp
jne .LBB1_4
jmp .LBB1_9
We can see a rather verbose assembly code with two loops. Now let's compare it with the assembly generated for the refactored version.
.LBB1_10:
mov rcx, rsi
mov rdx, qword ptr [rbp - 8]
add rbp, -8
mov qword ptr [rax], rsi
cmp rdx, 1
mov rsi, rdx
adc rsi, 0
imul rsi, rcx
imul r12, rdx
add rax, -8
cmp rbp, r13
jne .LBB1_10
Oh, that's pure beauty. One loop. Compact and efficient. Wow!
To those who aren't very familiar with the assembly language, it might seem strange that there isn't a conditional jump to implement this: sz == 0 ? 1 : sz.
The Now, we've reached the cherry on top: How does it work? 1 is subtracted from This is If All that's left is to perform two multiplications, decrement Here's how it works
The value of acc is stored in the RSI register. We'll need a copy of this value later, so it's stored in the RCX register.
mov rcx, rsi
RBP register contains the address used to iterate through the elements of the sizes array in reverse order. Here, we load the element into the sz variable (RDX) and shift the iterator by 8 bytes (the size of the int64_t element) to the left.
mov rdx, qword ptr [rbp - 8]
add rbp, -8
RSI is acc. Let's put the register in the strides array. RAX holds the element address. The jump to the previous element will be executed below (see add rax, -8).
mov qword ptr [rax], rsi
acc *= (sz == 0 ? 1 : sz) is executed and the RDX register(sz variable) is compared with 1.
cmp rdx, 1 // CF = is moved when sz - 1
RDX. If a jump occurs, the carry flag (CF) is set. This happens only when CF = 1 if RDX is 0.acc = sz. We overwrote the variable's value, but that's okay because we have a copy of it in RCX.
mov rsi, rdx // acc = sz
sz was 0, let's change it to 1. To do this, let's add the CF carry flag. If sz is not 0, then the carry flag is 0, and nothing will change. That's how the compiler avoided the conditional jump.
adc rsi, 0 // acc = acc + 0 + CF
RAX, and continue the loop.
imul rsi, rcx // acc = acc * {ACC copy in RCS register}
imul r12, rdx // ne = ne * sz
add rax, -8
cmp rbp, r13 // Do we reach the beginning of strides?
jne .LBB1_10 // If no, we'll get goto LBB1_10
Another way
Do we have another way? Yes, we have. I don't know whether I think about it or not. Unfortunately, after finishing my own refactoring, I made the mistake of immediately asking AI for alternative solutions.
DeepSeek suggestions weren't any better, and some were even worse than the original. For example, it optimized one of the code implementations by reversing the array.
// Wrap out strides
std::reverse(strides.begin(), strides.end());
But Claude Opus came up with an unusual implementation. It was noticed that we don't actually need two independent sequences of multiplications.
If all elements of the input array are non-zero, then at the end of the loop, acc == ne. And if even one element was zero, we can simply set ne to zero at the end. We return to the any_zero flag again, but in a smarter way. Using this idea, we can write the following code:
static TensorImpl make_contiguous_tensor(const std::vector<int64_t>& sizes) {
auto q = sizes.size();
std::vector<int64_t> strides(q);
int64_t acc = 1;
bool any_zero = false;
for (auto sz : std::ranges::views::reverse(sizes)) {
strides[--q] = acc;
acc *= (sz == 0 ? 1 : sz);
any_zero |= sz == 0;
}
const int64_t ne = any_zero ? 0 : acc;
....
}
Assembly code
.LBB1_7:
mov rdx, rsi
mov rsi, qword ptr [r13 - 8]
add r13, -8
mov qword ptr [rcx], rdx
test rsi, rsi
sete dil
cmp rsi, 1
adc rsi, 0
imul rsi, rdx
or al, dil
add rcx, -8
cmp r13, rbp
jne .LBB1_7
xor r13d, r13d
test al, 1
cmove r13, rsi
mov r14, r8
Will code that performs half as many multiplications run faster? My prediction—not necessarily. The processor can perform two unrelated multiplications simultaneously on different pipelines. I'm willing to bet that my version and Claude's version will run at almost the same speed.
Performance measurement
Let's see how these implementations compare in reality. Designing a fair benchmark took a bit of effort. First, I had to ensure the input data couldn't overflow a 64-bit signed integer, since signed overflow is undefined behavior and would invalidate the measurements. Second, to make the contest fair, the test cases alternated between arrays with and without zero elements. Finally, the input arrays had varying lengths.
I'm not claiming that the measurements are rigorous, but they represent a general idea. The speed of the first original algorithm is taken as the baseline. I used Clang with the -O2 and -m64 options.
- The original version with two loops: 1.
- My version with a single loop: 0.88.
- The Claude version with a single multiplication: 0.92.
The refactoring improved performance by about 10% on average. I was actually hoping for a 20–30% speedup, but 10% is still a nice result—especially considering that it came from simplifying the code rather than making it more complicated.
Wrap-up
Let's stop our little refactoring adventure here. Was it worth it?
From the perspective of improving this particular piece of code, I'd say no. Tests are generated. It's not critical that code is longer and slower than it could be.
It doesn't matter whether code is written by a human or generated by AI. One thing remains constant, someone must recognize what bad is and what good is. There must be experts who can spot the difference. Is the code good? Is it bad? Is the code fast? Is it secure?
Some people can accept just the fact that the code works—and who cares what's inside it. In some cases—for example, for building a prototype—this is reasonable. However, those who aren't interested in architecture, optimization, or security most likely simply don't see or understand the whole picture of developing and maintaining large software products. So, I think we'll likely see a lot of drawbacks on this topic.
AI is a powerful tool, but it's not a magic wand. When Claude suggested an alternative version of the algorithm to me, it was a good example of how AI can be used for research and to enhance human capabilities. But it can just as easily cause harm if used carelessly.
If you want to continue improving your skills as a C++ specialist and create reliable, secure software, I suggest you check out these useful e-books and webinars on our website:
Top comments (0)