Discussion: remove mp_set_long specialization #194

minad · 2019-04-04T10:59:46Z

any reasonably intelligent compiler will optimize the loop away

minad · 2019-04-04T11:12:41Z

Same for mp_get_long. But this is open for discussion. The compilers I am using optimize such things. And for more stupid compilers this will just result in one additional untaken branch. It is hardly worth the extra code.

nijtmans · 2019-04-04T11:26:03Z

Well, I think the Travis build will show why this specialization was necessary ;-)

minad · 2019-04-04T11:27:23Z

Ok, I am curious. I didn't test it. Is there an overflow issue or something?

nijtmans · 2019-04-04T11:27:58Z

Hint: the "<<" operator cannot shift more than the size of the data-type.

minad · 2019-04-04T11:30:45Z

Can it happen that DIGIT_SIZE > 8*sizeof(long)? Isn't shifting by the size of the datatype well defined?
Or is only shifting by size-1 well defined?

I think I just compiled the unspecialized version and my compiler did not warn. But maybe I used a different digit size. I will check.

nijtmans · 2019-04-04T11:36:04Z

Can it happen that DIGIT_SIZE > 8*sizeof(long)? Isn't shifting by the size of the datatype well defined?
Or is only shifting by size-1 well defined?

It can happen that sizeof(long)==4 and DIGIT_SIZE == 60. So, the answer is Yes, unfortunately.

nijtmans · 2019-04-04T11:37:50Z

bn_mp_set_long.c: In function ‘mp_set_long’:
bn_mp_set_long.c:15:1: warning: right shift count >= width of type [enabled by default]
MP_SET_XLONG(mp_set_long, unsigned long)
^
ar: creating libtommath.a
The command "./testme.sh ${BUILDOPTIONS}" exited with 128.

minad · 2019-04-04T13:04:04Z

@nijtmans Ok to merge then? With the shifting fix the generated code is equivalent to the specialized code.

any reasonably intelligent compiler will optimize the loop away

nijtmans

I'm OK with this change.

czurnieden · 2019-04-04T13:51:15Z

As far as I remember : we need to make everything explicit here at LTM, including operator precedence, which means parentheses around 8 * sizeof (b). Also: no magick numbers, use CHAR_BIT instead. Please think of all the innocent DPSs which need to squeeze much more bits into each byte! ;-)

minad · 2019-04-04T15:16:41Z

@sjaeckel ready to merge :)

sjaeckel · 2019-04-04T15:21:15Z

tommath_private.h

     while (b != 0u) {                                   \
        a->dp[x++] = ((mp_digit)b & MP_MASK);            \
-        b >>= DIGIT_BIT;                                 \
+        if (CHAR_BIT * sizeof (b) <= DIGIT_BIT) { break; } \


I was just writing this comment while you pushed the change...

my question is if it is really CHAR_BIT as suggested by @czurnieden or if 8 is correct...

I think of platforms like some DSP's where CHAR_BIT is 16 or 32

I guess the 8 must stay, but I'm not 100% sure yet and I've to leave...

Ok, probably. Since sizeof always counts bytes??

But we have sizeof*CHAR_BIT at other places too. Then we would have to remove all of those and replace with 8.

According to stackoverflow, sizeof is always bytes. https://stackoverflow.com/questions/11868211/does-sizeof-return-the-number-of-bytes-or-the-number-of-octets-of-a-type-in-c

Therefore the usage of CHAR_BIT is wrong at many places in tommath. I can fix this in a separate PR if you wish.

Ah and bytes are implementation defined. Hehe. This is shit.

@sjaeckel Please merge as is and we resolve the CHAR_BIT question in a separate PR. If we want to resolve it...

Ah and bytes are implementation defined. Hehe. This is shit.

Oh, yes! *deep sigh*

To quote the standard (ISO/IEC 9899:2011 (E). I'm still too stingy to get the new one)

3.6
1 byte
addressable unit of data storage large enough to hold any member of the basic character set of the execution environment
2 NOTE 1 It is possible to express the address of each individual byte of an object uniquely.
3 NOTE 2 A byte is composed of a contiguous sequence of bits, the number of which is implementation-defined. The least significant bit is called the low-order bit; the most significant bit is called the high-order bit.
(emphasis mine)
But also

6.5.3.4 The sizeof and _Alignof operators
…
2 The sizeof operator yields the size (in bytes) of its operand, […]
4 When sizeof is applied to an operand that has type char, unsigned char, or signed char, (or a qualified version thereof) the result is 1.

So you can get the size of a byte with all standard compliant compilers where it is in CHAR_BIT. But what is with all of the non-standard compliant compilers? They are not supported, you said? Well, we really need a list of supported ones somewhere in a place where we can point at it if somebody wants to blame us for the problems one of the many crappy c-compilers out there gave them.

Ok at least CHAR_BIT=BYTES_BIT. Then it is correct as it is now :)

(size_t)XXX". The (size_t) cast doesn't add anything here.

nijtmans · 2019-04-04T20:07:41Z

Well, I see the construct "sizeof(yyy) * (size_t) CHAR_BIT" in a lot of places. To me the (size_t) cast doesn't add anything here: since sizeof() already returns a size_t, what's wrong with multiplying that to an "int", giving a size_t result? As I read the C-spec, usages of CHAR_BIT is correct here. It could be 8, 16 or whatever.

minad · 2019-04-04T20:11:11Z

@nijtmans While I don't have anything against you or other collaborators editing my PRs, I would prefer if you keep create a separate PR for the separate issue you observed. Nevertheless, I also didn't like these unnecessary size_t casts and would like to see them gone 👍

minad · 2019-04-04T20:15:11Z

@nijtmans Since I am messing with your code, I guess it is ok ;) I am just trying to ease it a bit for @sjaeckel such that he can quickly merge things step by step.

nijtmans · 2019-04-04T20:19:01Z

So, it looks like we are in full agreement here! ;-)

nijtmans · 2019-04-04T20:24:20Z

Since this PR already has 5 commits, I think @sjaeckel will want us to do this commit again anyway, to keep a clean history in GIT. Well, then we can split it, now we know how it should look like. But if @sjaeckel accepts it as-is that would be easier for us. For me 5 commits like this is acceptable.

Still not activated by default since demo.c has many issues. Furthermore there are issues in the system headers due to Wsystem-headers.

minad · 2019-04-04T20:48:50Z

@nijtmans I suggest to split this into two cleaned-up PRs. One for the mp_set_long specialization and the second one fixing the casts etc. Do you agree?

nijtmans · 2019-04-04T20:55:50Z

I agree with splitting this into 2 PR's. Thanks for the -Wconversion and -Wsign-conversion fixes. It looks like I was too rigid in the (size_t) removals ;-)

minad · 2019-04-04T20:57:46Z

Shall I do the split?

nijtmans · 2019-04-04T21:00:21Z

Yes, go ahead! I'll put my approval there!

minad · 2019-04-04T21:09:12Z

Closing in favour of #199 and #200

minad changed the title ~~remove mp_set_long specialization~~ Discussion: remove mp_set_long specialization Apr 4, 2019

minad requested a review from nijtmans April 4, 2019 13:04

minad added 2 commits April 4, 2019 15:11

remove mp_set_long specialization

f38e56c

any reasonably intelligent compiler will optimize the loop away

try to fix the shifting issue

7c5e889

minad force-pushed the simplify-set-long branch from 5234d44 to 7c5e889 Compare April 4, 2019 13:11

nijtmans approved these changes Apr 4, 2019

View reviewed changes

use CHAR_BIT instead of 8

b224265

sjaeckel reviewed Apr 4, 2019

View reviewed changes

minad and others added 2 commits April 4, 2019 17:21

parens

cb1c065

Use "XXX * sizeof(yyy)" consistantly in stead of "sizeof(yyy) *

4f38cfc

(size_t)XXX". The (size_t) cast doesn't add anything here.

minad added 2 commits April 4, 2019 22:36

add back some size_t to fix -Wsign-conversion

95af1d8

fix some Wconversion and Wsign-conversion issues

3854d04

Still not activated by default since demo.c has many issues. Furthermore there are issues in the system headers due to Wsystem-headers.

minad mentioned this pull request Apr 4, 2019

Activate Wconversion and Wsign-conversion #198

Closed

This was referenced Apr 4, 2019

remove mp_set_long specialization #199

Merged

remove unnecessary size_t casts, fix Wconversion/Wsign-conversion issues #200

Merged

minad closed this Apr 4, 2019

sjaeckel deleted the simplify-set-long branch April 5, 2019 07:45

Discussion: remove mp_set_long specialization #194

Discussion: remove mp_set_long specialization #194

Uh oh!

Conversation

minad commented Apr 4, 2019

Uh oh!

minad commented Apr 4, 2019

Uh oh!

nijtmans commented Apr 4, 2019

Uh oh!

minad commented Apr 4, 2019

Uh oh!

nijtmans commented Apr 4, 2019

Uh oh!

minad commented Apr 4, 2019

Uh oh!

nijtmans commented Apr 4, 2019

Uh oh!

nijtmans commented Apr 4, 2019

Uh oh!

minad commented Apr 4, 2019

Uh oh!

nijtmans left a comment

Choose a reason for hiding this comment

Uh oh!

czurnieden commented Apr 4, 2019

Uh oh!

minad commented Apr 4, 2019

Uh oh!

sjaeckel Apr 4, 2019

Choose a reason for hiding this comment

Uh oh!

minad Apr 4, 2019

Choose a reason for hiding this comment

Uh oh!

minad Apr 4, 2019

Choose a reason for hiding this comment

Uh oh!

minad Apr 4, 2019

Choose a reason for hiding this comment

Uh oh!

minad Apr 4, 2019

Choose a reason for hiding this comment

Uh oh!

minad Apr 4, 2019

Choose a reason for hiding this comment

Uh oh!

czurnieden Apr 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minad Apr 4, 2019

Choose a reason for hiding this comment

Uh oh!

nijtmans commented Apr 4, 2019

Uh oh!

minad commented Apr 4, 2019

Uh oh!

minad commented Apr 4, 2019

Uh oh!

nijtmans commented Apr 4, 2019

Uh oh!

nijtmans commented Apr 4, 2019

Uh oh!

minad commented Apr 4, 2019

Uh oh!

nijtmans commented Apr 4, 2019

Uh oh!

minad commented Apr 4, 2019

Uh oh!

nijtmans commented Apr 4, 2019

Uh oh!

minad commented Apr 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

czurnieden Apr 4, 2019 •

edited

Loading