479501 - TM: using bool, not JSBool, for the return type of fastcall builtins

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Currently there are 3 boolean fastcall builtins in SM: js_EqualString, js_ArrayCompPush and js_Array_dense_setelem. They use JSBool as the return type. But this can potentially generates suboptimal code since the code calls js_EqualString from many places including a usage like:

bool cond = js_EqualString().

MSVC rightfully warns about this usage (see bug 477706) as it requires explicit code to normalize the result of js_EqualString into 0 and 1.

So it would be nice to change all the boolean builtins to use bool, not JSBool, for their return type.

Comment 1

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

That would require a nanojit change, since the ABI doesn't work correctly on Windows with bool-returning builtins.  See the comment at the top of jsbuiltins.cpp.

Comment 2

[Igor Bukanov]

(In reply to comment #1)
> See the comment at the top of jsbuiltins.cpp.

The comment just says:

 NB: bool FASTCALL is not compatible with Nanojit's calling convention usage. 

But exactly is the problem?

Comment 3

[Andreas Gal :gal]

Its an abi difference between Windows and mac/linux gcc the way booleans are represented. Nanojit really just supports "int32_t", which works as boolean for gcc, but not for Windows. Windows passes booleans in an 8-bit register if I remember correctly.

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> Its an abi difference between Windows and mac/linux gcc the way booleans are
> represented. Nanojit really just supports "int32_t", which works as boolean for
> gcc, but not for Windows. Windows passes booleans in an 8-bit register if I
> remember correctly.

Yes, that's it. Vlad and I saw al and were horrified (I flashed back to the early '80s ;-).

Let's not mess with this now.

/be

Comment 5

[Igor Bukanov]

(In reply to comment #4)
> Let's not mess with this now.

But what shall be done with js_EqualString and the current code in jstrace.cpp that assigns its result to bool? Should it be changes to use JSBool instead so MSVC would not need to normalize the function result into false|true?

Comment 6

[Jeff Walden [:Waldo]]

Explicit C++-style casts, as I understand it, may prevent that, but check the assembly to be sure.

Comment 7

[Igor Bukanov]

Jeff Walden wrote in comment #6:
> Explicit C++-style casts, as I understand it, may prevent that, but check the
> assembly to be sure.

The casts do not help as they simply suppresses the warnings. The issue is that functions using bool and int as their return types would have different ABI forcing the compiler to insert the conversion code when one calls another. The following example demonstrates what MSVC does at -O2 optimization level (the assembly includes only the differences):

int foo_int(void* p);
bool foo_bool(void* p);

bool int_to_bool(void *p)
{
	return foo_int(p);

00022	e8 00 00 00 00	 call	 ?foo_int@@YAHPAX@Z	; foo_int
00027	83 c4 04	 add	 esp, 4
0002a	85 c0		 test	 eax, eax
0002c	0f 95 c0	 setne	 al

}

bool int_to_bool_with_cast(void *p)
{
	return static_cast<bool>(foo_int(p));
00022	e8 00 00 00 00	 call	 ?foo_int@@YAHPAX@Z	; foo_int
00027	83 c4 04	 add	 esp, 4
0002a	85 c0		 test	 eax, eax
0002c	0f 95 c0	 setne	 al
}

bool bool_to_bool(void *p)
{
	return foo_bool(p);
  00022	e8 00 00 00 00	 call	 ?foo_bool@@YA_NPAX@Z	; foo_bool
  00027	83 c4 04	 add	 esp, 4
}

int bool_to_int(void *p)
{
	return foo_bool(p);
00022	e8 00 00 00 00	 call ?foo_bool@@YA_NPAX@Z; foo_bool  
00027	83 c4 04	 add	 esp, 4
0002a	0f b6 c0	 movzx	 eax, al
}

int bool_to_int_with_cast(void *p)
{
	return static_cast<int>(foo_bool(p));
00022	e8 00 00 00 00	 call	 ?foo_bool@@YA_NPAX@Z	; foo_bool
00027	83 c4 04	 add	 esp, 4
0002a	0f b6 c0	 movzx	 eax, al
}

int int_to_int(void *p)
{
	return foo_int(p);
00022	e8 00 00 00 00	 call	 ?foo_int@@YAHPAX@Z	; foo_int
00027	83 c4 04	 add	 esp, 4

}


So with or without casts there is a tax to do bool<->int conversions. This means that the code like (see http://hg.mozilla.org/tracemonkey/file/00d554e3db29/js/src/jstracer.cpp#l5217 )

static bool
evalCmp(LOpcode op, JSString* l, JSString* r)
{
    if (op == LIR_feq)
        return js_EqualStrings(l, r);
    return evalCmp(op, js_CompareStrings(l, r));
}

is suboptimal because js_EqualStrings returns JSBool, not bool. Now, one cannot change js_EqualStrings to use bool, not JSBool, as the traced code calls the function and does not support MSVC ABI. Which leaves two choices. 

One is do nothing and keep the code and MSVC warnings about the need for conversion code. Another is to stop using bool due to brokenness of the compiler on the most important platform (note that using true/false is still OK).

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Having a hard time understanding if this is still relevant now that tracejit is gone.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Igor, ping?

Comment 10

[Igor Bukanov]

All the relevant functions are either return bool nowdays or are refactored into something else.