Thread overview
bug report x86-64 code: je / jbe
Jun 14, 2023
Cecil Ward
Jun 16, 2023
Iain Buclaw
Jun 18, 2023
Cecil Ward
Jun 19, 2023
Iain Buclaw
Jun 26, 2023
Iain Buclaw
June 14, 2023
I have just noticed a bug in the latest release of GDC that targets x86-64. For example GDC 12.3 and above versions too, running on X86-64, targeting self. This was built with:
 -O3 -frelease -march=alderlake

Generated code is:

   je L1
   jbe L1

    …
    …
L1:  ret

The probably reason for this is my use of the GDC built in for indicating whether conditional jumps are likely or unlikely to be taken. I wrote a trivial routine likely() and unlikely() and used this as follows:

public
T pow(T, exp_ui_t )( in T x, in exp_ui_t p ) pure @safe nothrow @nogc
    	if ( is ( exp_ui_t == ulong ) || is ( exp_ui_t == uint ) )
in	{
    static assert( is ( typeof( x * x ) ) );
    assert( p >= 0 );
    }
out ( ret ) {
    assert( ( p == 0 && ret == 1 ) || !( p == 0 ) );
    }
do
    {
    if ( unlikely( p == 0 ) ) return 1;
    if ( unlikely( p == 1 ) ) return x;

    /*
    if ( unlikely( x == 0 ) )	// fast-path opt, unnecessary
    	return x;
    if ( unlikely( x == 1 ) )	// fast-path opt, unnecessary
    	return x;
    */
  	
    T s = x;
    T v = 1;
  	
    for ( exp_ui_t i = p; i > 1; i >>= 1 )
	{
	v = ( i & 0x1 ) ? s * v : v;
	s = s * s;
	}
   //assert( p > 1 && pow( x, p ) == ( p > 1 ? x * pow( x, p-1) : 1) );
    return v * s;
    }

pragma( inline, true )
private
bool builtin_expect()( in bool test_cond, in bool expected_cond )  pure nothrow @safe @nogc
{
    version ( LDC )
    	{// ldc.intrinsics.llvm_expect - didi not seem to work when tested in LDC 1.22
    	import ldc.intrinsics : llvm_expect;
    	return cast(bool) llvm_expect( test_cond, expected_cond );
    	}
    version ( GDC )
    	{
    	import gcc.builtins : __builtin_expect;
    	return cast(bool) __builtin_expect( test_cond, expected_cond );
    	}
    return test_cond;
    }


pragma( inline, true )
public
bool likely()( in bool test_cond ) pure nothrow @safe @nogc
/* Returns test_cond which makes it convenient to do assert( unlikely() )
 * Also emulates builtin_expect's return behaviour, by returning the argument
 */	{
    return builtin_expect( test_cond, true );
    }


pragma( inline, true )
public
bool unlikely()( in bool test_cond ) pure nothrow @safe @nogc
/* Returns test_cond which makes it convenient to do assert( unlikely() )
 * Also emulates builtin_expect's return behaviour, by returning the argument
 */
    {
    return builtin_expect( test_cond, false );
    }
// ~~~ module likely - end.

====

This is not the whole of this .d file, I can of course give you the whole lot if you desire. I inspected the result in Matt Godbolt’s compiler explorer website godbolt.org.

An aside: LDC:: I need to look at LDC’s llvm_expect to see if it is controlling the branches the way I wish. Does anyone know if llvm_expect has any problems?
June 16, 2023
On Wednesday, 14 June 2023 at 12:35:43 UTC, Cecil Ward wrote:
> I have just noticed a bug in the latest release of GDC that targets x86-64. For example GDC 12.3 and above versions too, running on X86-64, targeting self. This was built with:
>  -O3 -frelease -march=alderlake
>

What leads you to believe that it is buggy?

> Generated code is:
>
>    je L1
>    jbe L1
>

What I see is the first instruction is going to relate to this condition.

>     if ( unlikely( p == 1 ) ) return x;

Then the next instruction is the condition in the following for-loop.

>     for ( exp_ui_t i = p; i > 1; i >>= 1 )

Redundant jump? Yes, arguably. Leads to wrong runtime? Doesn't look that way.
June 18, 2023
On Friday, 16 June 2023 at 13:12:06 UTC, Iain Buclaw wrote:
> On Wednesday, 14 June 2023 at 12:35:43 UTC, Cecil Ward wrote:
>> I have just noticed a bug in the latest release of GDC that targets x86-64. For example GDC 12.3 and above versions too, running on X86-64, targeting self. This was built with:
>>  -O3 -frelease -march=alderlake
>>
>
> What leads you to believe that it is buggy?
>
>> Generated code is:
>>
>>    je L1
>>    jbe L1
>>
>
> What I see is the first instruction is going to relate to this condition.
>
>>     if ( unlikely( p == 1 ) ) return x;
>
> Then the next instruction is the condition in the following for-loop.
>
>>     for ( exp_ui_t i = p; i > 1; i >>= 1 )
>
> Redundant jump? Yes, arguably. Leads to wrong runtime? Doesn't look that way.

Completely agree with Iain, it’s not incorrect code, I wasn’t intending to suggest that. I’d just say suboptimal, and not the very best code generation possible.
June 19, 2023
On Sunday, 18 June 2023 at 02:55:32 UTC, Cecil Ward wrote:
> On Friday, 16 June 2023 at 13:12:06 UTC, Iain Buclaw wrote:
>>
>> Redundant jump? Yes, arguably. Leads to wrong runtime? Doesn't look that way.
>
> Completely agree with Iain, it’s not incorrect code, I wasn’t intending to suggest that. I’d just say suboptimal, and not the very best code generation possible.

By the way, I suspect it's related to this problem report.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96435

Taken from upstream DMD comment (I suspect is still not fixed in either DMD, possibly LDC too but haven't checked).

https://issues.dlang.org/show_bug.cgi?id=20148#c3

Essentially, every read of a boolean gets turned into `(*(ubyte*)&b) & 1`.

This *could* be limited to `@safe` code, or just accessing field members of a union I suppose.
June 26, 2023
On Monday, 19 June 2023 at 09:54:32 UTC, Iain Buclaw wrote:
> On Sunday, 18 June 2023 at 02:55:32 UTC, Cecil Ward wrote:
>> On Friday, 16 June 2023 at 13:12:06 UTC, Iain Buclaw wrote:
>>>
>>> Redundant jump? Yes, arguably. Leads to wrong runtime? Doesn't look that way.
>>
>> Completely agree with Iain, it’s not incorrect code, I wasn’t intending to suggest that. I’d just say suboptimal, and not the very best code generation possible.
>
> By the way, I suspect it's related to this problem report.
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96435
>

Fix has been committed in time for 13.2 release.