Bug in divmodhi4(), plus poor inperformant code

[Stefan Kanthak]

"Segher Boessenkool" <segher@kernel.crashing.org> wrote:

> On Wed, Dec 05, 2018 at 02:19:14AM +0100, Stefan Kanthak wrote:
>> "Paul Koning" <paulkoning@comcast.net> wrote:
>> 
>> > Yes, that's a rather nasty cut & paste error I made.
>> 
>> I suspected that.
>> Replacing
>>     !(den & (1L<<31))
>> with
>>     (signed short) den >= 0
>> avoids this type of error: there's no need for a constant here!
>> 
>> JFTR: of course the 1L should be just a 1, without suffix.
> 
> "int" can be 16 bits only, I think?

This function works on short int, which can only be 16 bits.

> If you change to 15 you can remove the L, sure.
> 
>> > But if the 31 is changed to a 15, is the code correct?
>> > I would think so.
> 
> I think so, too.
> 
>> > For optimization I'd think that an assembly language
>> > version would make more sense, and a few targets do that.
>> 
>> Moving 2 of 3 conditions from the loop is not an optimisation,
>> but a necessity!
> 
> The compiler can optimise things quite well.

It but doesn't, see <https://godbolt.org/z/e3CUY4>: lines 15 to 20
are inside the loop, line 6 and 9 to 11 skip the loop.

>> In other words: why test 3 conditions in every pass of the
>> loop when you need to test only 1 condition inside the loop,
>> and the other 2 outside/before the loop?
> 
> Maybe the code is easier to read this way.  Maybe it doesn't matter for
> performance.  Maybe no one cared, this routine is for correctness anyway,

No, no & yes, yes: it's not very likely that it will really be called.
OTOH, an unsuspecting kid may find and copy it, assuming that a routine
shipped with GCC can't be THAT bad.-P
Or the (almost) identical
<https://github.com/gcc-mirror/gcc/blob/master/libgcc/udivmodsi4.c> ...

> any target that cares for performance will do an asm version, or (partly)
> inline this even.
> 
> Send a patch if you want to see it changed :-)

I saw my last PDP-11 more than 30 years ago!

regards
Stefan