sys: util: add a sign_extend function

[fabiobaltieri]

Pretty much the same as https://elixir.bootlin.com/linux/latest/source/include/linux/bitops.h#L186, figured I would not use the 32 suffix since it works fine for smaller types as well but let me know what you think.

---

Add a sign_extend function, same as the Linux sign_extend32 one. This is useful for sign extending values smaller than 32 bits.

[cfriedt]

@fabiobaltieri - would it be ok to request a few unit tests for this function?

[fabiobaltieri]

@fabiobaltieri - would it be ok to request a few unit tests for this function?

Yeah good point, I'll add some tomorrow, back to draft for now.

[andyross]

Seems very reasonable. FWIW, worth noting that various architectures do this in various ways as a single-instruction primitive (or less, x86 can fold it into a load) instead of a double shift. Are we confident existing compilers optimize that correctly or should we worry about having an override in the toolchain layer? I guess one assumes that if this is how linux does this then gcc and clang are known to get it right?

[andyross]

Might be worth considering redefining as a macro with some __typeof__() magic to work with arbitrary input word sizes. Because you just know that someone is going to ask for a sign_extend_64().

[fabiobaltieri]

Yeah I tried something like

#define SIGN_EXTEND(value, index) \
       (__typeof__(value))(value << (NUM_BITS(value) - 1 - index)) >> (NUM_BITS(value) - 1 - index)

But then realized that I need the signed version of the type, not the same type, for that to work. :-) If you have any idea... meanwhile I've added a 64 bit variants to anticipate that, Linux has it so why not.

[fabiobaltieri]

• Added a 64 bit variant and some unit tests

Are we confident existing compilers optimize that correctly or should we worry about having an override in the toolchain layer? I guess one assumes that if this is how linux does this then gcc and clang are known to get it right?

Wow good question, I'd be shocked if there's a chance to optimize it further and it's not done in Linux, my code looks something like this on ARM:

575             return (int32_t)(value << shift) >> shift;
   0x000000c8 <+128>:   sbfx    r4, r4, #0, #12

So it looks like it's doing the right thing.

[fabiobaltieri]

Funny I found my bug in my driver while trying to figure why that optimization was only showing up once while I was expecting it to be there twice, thanks @andyross. :-)

[cfriedt]

LGTM. If there happens to be another rev for some reason, it would be best to split it into one code commit and one test commit.

[peter-mitsis]

Two questions ...

1. Is it worthwhile adding an assert check to verify the range of the index parameter? Recall that shifting by more bits than the type allows leads to undefined behavior. For example, on x86 value >> 34 is equivalent to value >> 2 if value is 32 bits wide. However, on ARM, IIRC, the same expression would evaluate to 0.
2. Should the index parameter be an unsigned type? It does not make much sense to have a negative bit index.

[fabiobaltieri]

@peter-mitsis sure, can't think of any reason against either, added few asserts, switched index to uint8_t

[cfriedt]

@fabiobaltieri - if you are re-spinning anyway, would it be possible to separate the implementation in the 1st commit, and the test in a 2nd commit?

[fabiobaltieri]

@fabiobaltieri - if you are re-spinning anyway, would it be possible to separate the implementation in the 1st commit, and the test in a 2nd commit?

Yes sorry forgot, split.

About the assert, turns out those are unhappy with some tests, I dropped it again for now will look into it later

[fabiobaltieri]

nvm fixed the assert issue and reintroduced again, test split on its own pr