2

I've this template function

template <typename T, typename It, std::enable_if_t<std::is_integral<T>::value, int> = 0>
inline T decode(It &it) {
    static_assert(std::is_same<typename std::iterator_traits<It>::value_type, std::uint8_t>::value, "invalid");
    T* v_p = reinterpret_cast<T*>(&*it);
    it += sizeof(T);
    return *v_p;
}

that is used to decode integers from a raw pointer. The function can be used with any type that has iterator traits, i.e. either pointers to std::uint8_t or iterators to std::uint8_t STL containers with iterators that meet LegacyContiguousIterator requirements.

The function works, but I'm not sure about the performance of the call to &*it when it is a pointer. The operators are needed to get the pointer from an iterator, as explained in this answer, but it seems overkill for POD pointers. Is the compiler allowed to just drop the operations, or it is better to write a specialization for pointers

Giovanni Cerretani
  • 1,693
  • 1
  • 16
  • 30
  • `it += sizeof(T)` is **highly** suspicious. Iterator increments move to the next element, not the next byte. But `sizeof(T)` is a size in bytes. The `static_assert` helps, but an `enable_if` would be more appropriate. – MSalters Jan 24 '20 at 13:36

3 Answers3

6

For a pointer then yes due to the as-if rule allows it.

But in this case, if it isn't a pointer but instead an object with an overloaded dereference operator then the compiler can't do that.

Some programmer dude
  • 400,186
  • 35
  • 402
  • 621
  • 1
    Even more: For a pointer, `&*` always must compile down to a noop. Simply because once you load the pointee value into the CPU (`*`), *it does not have an address anymore*. So, for `&*` to be meaningful, the dereferenciation step must be omitted, and the address is the nothing else but the current pointer's value. – cmaster - reinstate monica Jan 24 '20 at 09:17
  • Do `std::vector` or `std::array` iterators overload `operator&`? – Giovanni Cerretani Jan 24 '20 at 09:36
  • 1
    @GiovanniCerretani No, but all iterator classes overload the dereference operator `*` (which includes the `std::vector` iterator). – Some programmer dude Jan 24 '20 at 09:37
  • @Someprogrammerdude thanks, of course I meant `operator*`. – Giovanni Cerretani Jan 24 '20 at 09:41
  • 1
    @cmaster-reinstatemonica: That's not what `*` does. `*` converts a pointer into an lvalue. An lvalue can have its address taken. – MSalters Jan 24 '20 at 10:02
  • @MSalters My comment was on a relatively low level, where I actually think about machine instructions. On the higher level where the concept of an `lvalue` lives, it is important to realize that the conversion of a pointer to an `lvalue` is only a *conceptual* change. It's just telling the compiler: "Now I'm not talking about the address stored in this pointer anymore, but rather about the object behind that pointer." This conceptual change does not compile into anything in and off itself, only further use of the `lvalue` may trigger a memory load/store to be emitted by the compiler. – cmaster - reinstate monica Jan 24 '20 at 10:35
  • @cmaster-reinstatemonica: There is an important change though. Pointers can be null, but an lvalue must refer to an actual object. That's one reason why `&*` isn't a noop; it's Undefined Behavior on a null pointer. – MSalters Jan 24 '20 at 10:42
  • [Here](https://godbolt.org/z/CAPmQL) I tried to compare the compiler output for `std::vector`, `std::array` and pointer. Except for arrays (trivial solution, no heap allocation), in the vector and pointer versions the output are almost identical except for the value initialization of the `std::vector` elements. The call to `operator*` seems omitted. – Giovanni Cerretani Jan 24 '20 at 10:44
  • @MSalters An undefined behavior that is virtually always implemented as a noop. I mean, the whole point of allowing undefined behavior is to allow for code *not* to be emitted. The fact that `&*(sometype*)NULL` is UB simply allows the compiler to remove other code, should the compiler be able to prove the UB. If the compiler does not recognize the UB, it will emit what it will emit for the defined case, and that is nothing. (You seem to be looking at this with more of a language lawyer perspective. My focus is more on the actual translation results. Both are useful views imho.) – cmaster - reinstate monica Jan 24 '20 at 12:52
  • @cmaster-reinstatemonica: UB allows more optimizations than that. For instance, it can allow reordering, which means that the compiler actually moves code. The idea is that a load takes time, and you want to start it early. Knowing that you won't hit an accidental null pointer dereference might allow the load to start a few instructions early. Modern compilers understand CPU pipelines and do this kind optimization. – MSalters Jan 24 '20 at 13:04
  • @MSalters I know. But how is that relevant to whether `&*` compiles as a noop or not? – cmaster - reinstate monica Jan 24 '20 at 13:26
  • @cmaster-reinstatemonica: What you're overlooking is that compilers don't compile from `&*` straight to assembly. They convert the C++ code to an intermediate representation, shuffle that around for optimization, and turn the result into assembly. It's entirely possible that the `&*` pair of operations doesn't end up paired prior to the conversion to assembly. – MSalters Jan 24 '20 at 13:32
  • @MSalters And what you are overlooking is, that `&*` used on a pointer never has any necessary observable effect. It would be a clear performance bug for a compiler to emit any code for it. If the compiler does shuffle stuff around, tearing the `&*` apart, and ends up actually emitting code for the `*` operator, then that's always due to some *other* use of the `*pointer` subexpression, not to the `&*` being present in the code. – cmaster - reinstate monica Jan 24 '20 at 13:55
  • @cmaster-reinstatemonica: I'm not overlooking that. Your assumption is wrong; the compiler may introduce a speculative load when you write `&*`. This speculative load is safe because the pointer must be valid because of C rules. The load is a performance optimization in a pipelined CPU, because it doesn't block the next unrelated instruction and hides the latency of a later load. – MSalters Jan 24 '20 at 14:42
  • @MSalters A speculative load is only warranted if the loaded value can actually end up being used. With `&*` it can't. As such, adding such a load for `&*` would be a pessimisation. In order for the compiler to have *any* reason to actually add a load instruction, there must also be a *use* of the value somewhere (i.e. `pointer->member` or `pointer[i]`). And that use must be supplied by some other expression than `&*pointer`. Don't get me wrong, the standards definitely allow the load. But there is simply no point in adding it. And if it is added, that's a performance bug. – cmaster - reinstate monica Jan 24 '20 at 15:02
0

If it is a pointer, then &* will be a noop.

But, in any case, whenever you claim something like:

I'm not sure about the performance of [...]

You really need to be able to measure the difference. If you cannot, you are probably worrying about nothing.

Acorn
  • 24,970
  • 5
  • 40
  • 69
  • You forget evil class with overloaded unary `operator&`. – Jarod42 Jan 24 '20 at 10:06
  • @Jarod42 Hm... for that to happen we would need to specialize `std::iterator_traits` for a particular pointer so that the standard pointer specialization doesn't kick in, because otherwise given OP claims `It` is a pointer, and given the `static_assert` in the question, then `It` has to be `std::uint8_t *`. I think. – Acorn Jan 24 '20 at 10:32
  • indeed, `std::uint8_t *` is fine. but without the `static_assert`, `MyClass *` might be problematic. – Jarod42 Jan 24 '20 at 11:37
0

In this case is wrong, because you use an iterator that isn't a pointer but only an object that have a dereference operator, so the basic rule valid for pointers can't be applied in this case.

For more information see the AS-IF-RULE

Zig Razor
  • 3,381
  • 2
  • 15
  • 35
  • We don't *know* if `it` is an iterator. In fact `it += sizeof(T)` indicates that it's not supposed to be. – Some programmer dude Jan 24 '20 at 09:21
  • Exactly. With an iterator, there is no guarantee that the bytes are actually consecutive in memory, so the `reinterpret_cast<>` may easily invoke undefined behavior. I would write such a function using a `char*`, or just do the `reinterpret_cast<>` where I need it. That's clearer than hiding the cast away in a templated function. – cmaster - reinstate monica Jan 24 '20 at 09:22
  • great observation... i do not note these particular – Zig Razor Jan 24 '20 at 09:22
  • 1
    @Someprogrammerdude The fact that `+=` is used on the iterator does not mean that the bytes are actually stored consecutively. All the `+=` forces is, that the iterator must be a random access iterator. – cmaster - reinstate monica Jan 24 '20 at 09:23
  • @cmaster-reinstatemonica thanks for the particular. I'm going to edit the question changing the iterator type to **LegacyContiguousIterator** requirements. – Giovanni Cerretani Jan 24 '20 at 09:28