Add a unsigned version of Convert16BitSamples() in Oscilloscope class

[fredzo]

Some instruments (like Batronix's Magnova) return 16 bit data in the form of unsigned 16 bit integers rather than signed 16 bit integers.
To work around this, a custom Convert16BitSamples() method has been created in the Magnova driver.
We need to create a generic ConvertUnsigned16BitSamples() (unsigned version of Convert16BitSamples()) in Oscilloscope class for use in future drivers that work with 16 bit unsigned int.

[diggit]

Can you provide more details? Transport typically returns buffer of bytes. Driver has to do the per sample conversion math anyway. What would this generic Convert16BitSamples solve?

I agree, that Some generic (possibly templated?) offset/gain sample conversion tools would be handy.

I checked your fork with magnova-driver branch (please reference it next time)

// const std::vector<uint8_t>& data

const uint8_t* ptr = data.data() + 32;
const uint16_t* wdata = reinterpret_cast<const uint16_t*>(ptr);

Such reinterpret cast is not standard compliant, though it may work with most compilers.

Valid approach in this case is to use std::memcpy (which will get optimized out).

[azonenberg]

We have standard code for doing int8 to float conversion in the Oscilloscope class so we can optimize it with vector instructions etc and not have to duplicate this code in each driver. We also have reusable GPU implementations of the conversion.

There's no reason to have this code in a driver class when it can be refactored into the base class so it'll be usable by any future instruments that take uint16 samples as well.

[azonenberg]

See https://github.com/ngscopeclient/scopehal/blob/master/scopehal/Oscilloscope.cpp#L965 for example

[diggit]

Good to know, I was not aware of these int8/int16 conversion functions.

As @fredzo wrote, generic u16 sample conversion functions in core would be also handy and i16 version will not cut it.

@azonenberg my point about reinterpret_cast also applies there

scopehal/scopehal/Oscilloscope.cpp

Line 1049 in 76c1b1d

__m256i raw_samples = _mm256_loadu_si256(reinterpret_cast<const __m256i*>(pin + k));

reinterpret_cast has very strict usage rules in C++ and this is not among allowed uses.

[azonenberg]

The cast should be OK here because this is an assembly intrinsic that compiles to a vmovdqu instruction which has no specific alignment restrictions. So while it may be UB in the generic C++ sense, by definition you are running on x86 if you are compiling this code and the x86 semantics are well defined.

[azonenberg]

If you can point me to a compiler which will do anything wrong with this code, I'd be very curious (especially since intrinsics are usually special-cased in implementations for various reasons). I'm not aware of a better way to structure this code short of literally writing it in assembly rather than using intrinsics, which makes it a lot less readable.

This should end up compiling to vmovdqu ymmFOO, ymmword ptr [pin + k*2] which it does in fact do on every compiler I've looked at the output of.

[azonenberg]

See https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html#text=_mm256_loadu_si256&ig_expand=4108 where the intrinsic explicitly states no alignment requirements

[azonenberg]

Note also that my reading is that dereferencing, not creating, a misaligned pointer is where UB comes in. If you create the misaligned pointer and pass to an intrinsic, then it's dereferenced outside the C++ abstract machine, I believe this is fully well defined.

[diggit]

If you can point me to a compiler which will do anything wrong with this code...

Yeah, today compilers are quite careful, but when the code does not follow standard, bad thing may happen (UB). Sorry, that is my itch.

Note also that my reading is that dereferencing, not creating, a misaligned pointer is where UB comes in. If you create the misaligned pointer and pass to an intrinsic, then it's dereferenced outside the C++ abstract machine, I believe this is fully well defined.

This is interesting approach and I can't argue with that. Also in this case it is cast from array of i8 to other type which is array of i8, so it could be valid. That would need more experienced C++ guru than myself.

Anyway we brought this discussion quite far from original topic (maybe consider marking these comments as OT) 😅.