Add bounds_t with pre-aligned mins and maxs

[illwieckz]

This is not urgent at all. This is complementary to:

• qmath: micro-optimize the BoxOnPlaneSide function #1142

It also improves over:

• introduce plane_t struct with normal and dist members #1043

The BoxOnPlaneSide() function is known to be a hot spot, being called by multiple recursive functions. Right now we spend a lot of time in _mm_loadu_ps, we have to call sseLoadVec3Unsafe() explicitly because we can't guess if the data is aligned or not. This comes with multiple downside:

• _mm_loadu_ps is said to be slower than _mm_load_ps, and that fits my experience¹.
• The compiler doesn't optimize _mm_loadu_ps and will always call it if we ask for it explicitely, even if the data is already aligned, and by experience, even if no copy is needed.

So the idea is to introduce some nice bounds_t struct that uses aligned mins and maxs, same for cplane_t with an aligned normal. When doing that, we can write an explicit _mm_load_ps that is faster than _mm_loadu_ps, and most of the cases, because the compiler knows the data is const and already aligned, the compiler just removes the _mm_load_ps and process the data without any copy.

See also:

• Add bounds_t with pre-aligned mins and maxs Unvanquished/Unvanquished#3265

---

¹ Some times ago I tried to write optimized SSE code for some other functions, but the code was slower because of the explicit _mm_loadu_ps call. I even noticed copying an unaligned vec3 to an aligned vec3 before calling _mm_load_ps could make the code faster when the compiler notices the data is already aligned and skips the copy and calls _mm_load_ps (or even doesn't need to call it at all).

[illwieckz]

For now there is a bug somewhere I have not found yet. The bug can be seen by loading the plat23 map and looking to the left (some surfaces will disappear).

In my first iteration of the branch I made a stupid mistakes by passing the bounds_t by value instead of by reference, meaning functions modifying it would not modify it. I assume this is now fixed but I guess I left somewhere another mistake as stupid as that.

[illwieckz]

Note: the percentage of time spent is not reliable in those screenshots because I use a viewpos on some acid tubes spamming acid gas and the amount of particles is very variable.

Before (C), a lot of time is spent in sseLoadVec3Unsafe():

After (C), the first loads are just completely skipped:

Before (Asm), we see three movups instructions:

After (Asm), se see only two movaps instructions:

[illwieckz]

So, as I said, this is not urgent. But I would appreciate people re-reading what I did in hope someone finds the stupid mistake I may have done. The code change should be straightforward because it's basically rewriting with a different data layout, there is no algorithm change to be expected. Unfortunately this data struct is used in many places so the patch is massive.

There are many functions in CM not using the new structure yet that can be migrated, but that can be done later as the patch is already heavy as it is. Migrating other functions would be mostly code clean-up and only about code purity, it is not required to make it work. Though maybe it can also help the compiler to optimize in other places. For the same reasons, I only modified the game to be compatible with the new structs and shared functions, using some wrapping around the new struct, to keep the patch light on game side (porting the game to the new struct for the sake of purity can be done later as well).

[illwieckz]

I also added some constness to some function input, which may help the compiler to optimize a bit more some other places of the code. I doubt this extra-constness is the cause of the bug I'm facing because the compiler doesn't complain I'm writing to some const structs, so hopefully I only added some const keywords to structs that are only read and not written.

[illwieckz]

Here I forgot:

out->bounds.maxs[ 0 ] = LittleLong( in->maxs[ 0 ] );

[illwieckz]

That was the bug I was looking for.

[illwieckz]

So I reread everything and found the bug I was looking for. I missed the conversion of one line (I just did not re-added the new one, that's why there was no compile type error).

It works.

[illwieckz]

I found another bug affecting BSP movers like doors. One can test it in the Vega map. The doors disappear according to the viewing angle / point of view.

[DolceTriade]

Any fps difference?

[slipher]

I think you can make a drop-in aligned replacement for vec3_t like this:

struct alignas(16) alignedVec3_t : public std::array<float, 3> {}; 

This can be used in arrays so then you don't need bounds_t with the ugly at function.

[VReaperV]

This looks wrong. The previous variables in the struct add up to 6 bytes.

[illwieckz]

That was because of:

• Add bounds_t with pre-aligned mins and maxs Unvanquished/Unvanquished#3265 (comment)

But maybe I'm wrong with the values.

[VReaperV]

Sure, but it should be byte pad[ 14 ], rather than byte pad[ 12 ].

[VReaperV]

(small correction to my previous comment: they add up to 18 bytes, not 6)

[illwieckz]

I think you can make a drop-in aligned replacement for vec3_t like this:

struct alignas(16) alignedVec3_t : public std::array<float, 3> {}; 

This can be used in arrays so then you don't need bounds_t with the ugly at function.

Interesting, how such type can be used and where?

[slipher]

As a direct replacement for vec3_t. alignedVec3_t localBounds[ 2 ]; This would automatically work with indexing operations and functions like VectorCopy. But you would need a .data() or whatever to pass it to functions taking a float* (this includes functions with vec3_t parameter)

[illwieckz]

I like the idea of having have an alignedVec3_t but I don't see the benefit of making bounds_t an array of it. There is only a few usage of at(), while using .data() everywhere would be much verbose. Also I like the idea of explicit mins and maxs naming, I guess we may be able to do some mins() and maxs() and I tried to implement that with an alignedVec3_t but that because much more complex.

I just rewrote at() in a way it is now guaranteed to be branchless.

[slipher]

I like the idea of having have an alignedVec3_t but I don't see the benefit of making bounds_t an array of it.

I'm suggesting that no bounds_t is needed. As I wrote in the previous comment, it could be used like alignedVec3_t localBounds[ 2 ];

[illwieckz]

But I like having a explicit bounds_t! It being implemented the way I did it or doing it like using bounds_t = alignedVec3_t[2], I want to do it…

[slipher]

There are random cmake changes in here by mistake

[slipher]

That's undefined behavior, like the (&v.x)[i] thing with GLM.

[illwieckz]

But how can this be undefined if the struct layout is guaranteed? And if it isn't guaranteed, how can we use structs for defining file storage format or network packet formats if the layout of the data isn't guaranteed?

Is there a type in C/C++ that guarantees that alignas(16) vec4_t, alignas(16) vec4_t has an offset of sizeof(vec4_t) or 16?

[illwieckz]

Can this be solved with static assertion (if the compiler doesn't do as expected, don't let the code be compiled?).

[slipher]

It doesn't matter that the layout is guaranteed. It's illegal to use an index that goes outside the bounds of an array. (A non-array object is treated like an array of length 1.)

[illwieckz]

But this is not an index, this is a pointer. Pointer arithmetic isn't undefined.

[slipher]

OK I didn't explain it precisely. Let's cite people who can. https://en.cppreference.com/w/cpp/language/operator_arithmetic#Pointer_arithmetic

Or one would say that if you have float array[3], doing &array[0] + 2 isn't defined because that goes outside of the object (array[0]).

This is the case Otherwise, if P points to the ith element of an array object x with n elements, given the value of J as j, P is added or subtracted as follows: . J can be from 0 to 3.

If you do &array instead of &a[0], it's the case Otherwise, if P points to a complete object, a base class subobject or a member subobject y, given the value of J as j, P is added or subtracted as follows: . J can be 0 or 1. Note that if it is 1, the resulting pointer is a "pointer past the end" and is illegal to dereference, even if it would happen to have the same numerical value as a valid object.

[illwieckz]

So, is this defined?

vec_t* at( bool index )	{ return (vec_t*) this + ( index * 16 );	}

The pointer doesn't go past the end of the object this.

[illwieckz]

Well, that doesn't work, actually only the if (index) return maxs; return mins work:

https://godbolt.org/z/ETEqnG9Gv

What's curious is that the compilers (GCC, Clang) seem to produce the same code for all three functions, but produce garbage on the functions doing pointer arithmetic. Edit: In fact the computed offsets are wrong.

[VReaperV]

Edit: In fact the computed offsets are wrong.

The wrong offsets are the ones in your at() functions, i. e. it should be index * 4, rather than index * 16.

[VReaperV]

I've seen claims that, essentially, doing this would not be UB:
return (vec_t*) ( (char*) &mins[0] + ( index ? offsetof( boundsA_t, maxs ) : offsetof( boundsA_t, mins ) ) );
However, don't quote me on that, & it produces the same assembly anyway.