JIT: enable JIT on Linux - #1591

jianchun · 2016-09-16T23:19:10Z

Splitted into 4 commits.

JIT: enable JIT on Linux

Primary changes to JIT on Linux.

Custom calling convention
-------------------------

Calls to native helpers conform to Sys V AMD64 ABI.
    - Change all code to use correct arg registers.

Calls to JsMethod and native helpers that depend on
ARGUMENTS/RUNTIME_ARGUMENTS use custom calling convention:
    - Caller home arg registers onto stack.

Implemented all needed assembly thunks.

Unwind data
-----------

Emit .eh_frame data for dynamic interpreter thunks and all jit functions.
    - For less noise, the new code hides behind existing
        PrologEncoder/PData/XData.

Misc
----

ARGUMENTS/RUNTIME_ARGUMENTS:
    - Extended to support all runtime usages.
    - Limitation: known arg list must end with "callInfo".

Int32Math:
    - Use builtins to test signed int overflow. Original code does not work
        due to clang optimization.

TODO
    - AsmJs: missing ArrayBuffer out of bound access recovery.
    - Simd.

JIT: to compile on Linux

Noisy changes to compile native code gen code on Linux with clang.
Most changes are:
    - Fix missing parentheses around `&&` within `||` expressions
    - Fix `char16/_u` issues
    - Fix incorrect `inline`s

JIT: PAL related changes

PAL related changes to build JIT on Linux.
    - pal.h: bug fixes in BitScan.
    - Add missing APIs, enable Event.

JIT: build and test changes

Build:
    - Add backend files.
    - Enable --no-jit option, and as default on OSX (temporarily).

Test:
    - Add `dynapogo` test variant. Change `interpreted` test variant to
        create dynamic profiles and input to `dynapogo` test variant.
        To ensure the same profile file is used for these 2 variants,
        TestLoader (new) assigns each test case a unique test id, and uses
        the id to generate dynamic profile file name.
    - Add 'disable_jit` test variant and as default on OSX (temporarily).
    - Exclude 2 failing tests that need further work.

jianchun · 2016-09-16T23:55:06Z

@LouisLaf @curtisman @digitalinfinity @obastemur Please CR, thanks!

Update: Please hold on CR. OOP JIT is merged and has some significant conflict.

Update: Completed merging with OOP JIT. Please CR.

obastemur · 2016-09-19T19:52:36Z

        ;;

+    --no-jit)
+        NO_JIT="-DNO_JIT=1"


Can you make it like -DNO_JIT_SH=1 and under the CMake file,

if(NO_JIT_SH) unset(NO_JIT_SH Cache) set(NO_JIT 1) else() unset(NO_JIT Cache) endif()

This is needed to switch between interpreter / jit builds without dealing with folder cleanup.

Will try this, thanks!

This works, thanks! I'm using NO_JIT in place of NO_JIT_SH as NO_JIT is not needed afterwards.

nit:_SH suffix is useful for remembering the source of the variable though. The rest of the definitions from build.sh use the same suffix.

@obastemur Agreed, changed 3 variables (all added by me) to _sh suffix in latest update.

@jianchun Thanks

obastemur · 2016-09-21T14:14:00Z

+    }
+
+    // update length record
+    uword length = writer->Count() - beginOffset


Assert(sizeof(length) + beginOffset <= writer->Count()) ?

I don't see this assert to be very useful. Once Begin() was called this would hardly fail as the same uword type is written again at beginOffset. Maybe I can assert beginOffset != -1 to ensure Begin() was called?

obastemur · 2016-09-21T14:23:14Z

+
+
+#ifndef __APPLE__
+// BailOutRecord::BailOut(BailOutRecord const * bailOutRecord)


Is this intentionally left?

These 2 symbols are used as JMP targets in this file. Don't know why it works with these commented out. Not sure if OSX needs them. Can cleanup when enabling this for OSX.

But that part is ifndef __APPLE__ I don't think it has anything to do with OSX

Yes, originally I thought I needed them on Linux that's why they are under ifndef __APPLE__. I can remove ifndef and only keep the commented .externs.

Removed ifndef __APPLE__.

obastemur · 2016-09-21T14:24:51Z

+        static const RegNum s_argRegs[IntArgRegsCount] = {
+    #define REG_INT_ARG(Index, Name)  Reg ## Name,
+    #include "RegList.h"
+        };


undef REG_INT_ARG ?

Because this file is included many times and the file has default #defines, I added #undefs in the file itself.

obastemur · 2016-09-21T14:25:08Z

+            reg = Reg ## Name;      \
+            break;
+#include "RegList.h"
+        default:


~~undef REG_INT_ARG?~~

obastemur · 2016-09-21T14:28:21Z

+    *pResult = val + 1;
+    return val == INT32_MAX; // Overflow if val was int max
+#else
+    return __builtin_add_overflow(val, 1, pResult);


Is there a reason why we haven't use this for __APPLE__ too ?

AppleClang does not have this builtin (reported by CI).

Actually AppleClang has that. Don't know why CI complained.

Instead of making it ifdef __APPLE__ we better feature detect from clang.

#ifdef __has_builtin #if __has_builtin(__builtin_add_overflow)

Same applies to other builtin comments below

Thanks, will do that. Does this work on gcc?

Does this work on gcc?

Hopefully for GCC 6.0. We don't support GCC though. There are so many problems with GCC.

@obastemur what are those problems?

@DemiMarie not a small list. i.e. Clang has some useful flags for C/C++/ASM code that is initially developed for VC/VC++

obastemur · 2016-09-21T14:28:35Z

+    *pResult = val - 1;
+    return val == INT32_MIN; // Overflow if val was int min
+#else
+    return __builtin_sub_overflow(val, 1, pResult);


Is there a reason why we haven't use this for APPLE too ?

obastemur · 2016-09-21T14:36:50Z

-#define VA_LIST_TO_VARARRAY(vl, va, callInfo) Js::Var* va = (Js::Var*) vl;
+#define DECLARE_ARGS_VARARRAY(va, ...)                              \
+    va_list _vl;                                                    \
+    va_start(_vl, callInfo);                                        \


We better call va_end once we are done with it. Previously, I had cleaned up the va leaks. See #1507

This is Windows x64 only and the macro does not know when we are done with it (we cast _vl to Var* and using it).

obastemur · 2016-09-21T14:41:53Z

+
+//extrn ?GetStackSizeForAsmJsUnboxing@Js@@YAHPEAVScriptFunction@1@@Z: PROC
+//extrn ?UnboxAsmJsArguments@Js@@YAPEAXPEAVScriptFunction@1@PEAPEAXPEADUCallInfo@1@@Z : PROC
+// extrn ?BoxAsmJsReturnValue@Js@@YAPEAXPEAVScriptFunction@1@HNM@Z : PROC


nit: extra space

obastemur · 2016-09-21T14:43:35Z

-#define swprintf_s _snwprintf
-#define vsprintf_s _vsnprintf
-#define vswprintf_s _vsnwprintf
+// #define sprintf_s _snprintf


We might better remove them?

jianchun · 2016-09-24T04:08:12Z

@rajatd @obastemur @curtisman Updated, please review, thanks!

rajatd · 2016-09-27T01:10:18Z

 void
 Opnd::WriteToBuffer(_Outptr_result_buffer_(*count) char16 **buffer, size_t *count, const char16 *fmt, ...)
 {
-    va_list argptr = nullptr;


why remove the initialization?

va_list is unnecessarily a pointer type (don't recall exactly, likely can't be initialized to nullptr). It is initialized by va_start.

rajatd · 2016-09-27T21:48:05Z

 #endif

+#if ENABLE_NATIVE_CODEGEN
+#ifdef _WIN32


|| with _WIN64?

_WIN32 contains _WIN64 (https://msdn.microsoft.com/en-us/library/b0084kay.aspx):

_WIN32 Defined as 1 when the compilation target is 32-bit ARM, 64-bit ARM, x86, or x64. Otherwise, undefined. _WIN64 Defined as 1 when the compilation target is 64-bit ARM or x64. Otherwise, undefined.

rajatd · 2016-09-28T21:37:59Z

+        };
+
+        const int callArgCount = static_cast<int>(argCount) + this->helperCallArgsCount;
+        const int argRegs = min(callArgCount, static_cast<int>(IntArgRegsCount));


min(callArgCount, static_cast(IntArgRegsCount)); [](start = 28, length = 53)

XmmArgRegsCount is 8. If there were, in fact, 8 float arguments, wouldn't we be missing to home 2 of them in the below loop?

This actually only homes the int registers, corresponding to our runtime usage (mostly function, callInfo, Js::Var... which don't use float registers). I'm not sure of AsmJs/Simd requirements. This PR has some basic thunk assembly implementations targeting AsmJs unit tests. Simd not looked at yet.

rajatd · 2016-09-28T22:40:43Z

#ifdef _WIN64

change this to TARGET_64?

Refers to: lib/Backend/Encoder.cpp:689 in ded0397. [](commit_id = ded0397, deletion_comment = False)

rajatd · 2016-09-28T22:40:49Z

#if _WIN64

change this to TARGET_64?

Refers to: lib/Backend/BackwardPass.cpp:163 in ded0397. [](commit_id = ded0397, deletion_comment = False)

jianchun · 2016-09-28T22:47:14Z

change this to TARGET_64?

Sure. Maybe I can search for all _WIN64 usage -- likely they should all be TARGET_64?

rajatd · 2016-09-28T23:42:12Z

+    #include "RegList.h"
+        };
+
+        const int callArgCount = static_cast<int>(argCount) + this->helperCallArgsCount;


helperCallArgsCount [](start = 68, length = 19)

Can helperCallArgsCount be non-zero here? If yes, then the loop below could potentially overwrite some of the stack slots written to in the above while(argsLeft>0) loop. Please verify this and add asserts to make sure that only one of these loops run. Some comments about which loop is used for what kind of call would be nice too.

Yes, will test that. Thanks!

@rajatd I looked at this issue. Yes helperCallArgsCount can be non-zero. The reason is that many lowering code uses LowerCallArgs() to lower variadic JS runtime args, and LoadHelperArgument() to lower other "known args". So the full args length is actually their sum, while argCount > 0 is an indicator that we need variadic args support so we need to manually home args.

Your concern of manual homing args here overwriting above helperCallArgs stack slots could potentially happen... e.g. helperCallArgs == 7 so it has an arg at stack[0]. I can fix this by changing above /*isHelper*/true to argCount == 0 so that their assumptions are consistent.

Yes, using "argCount == 0" as the argument to GetArgSlotOpnd should ensure correctness.

In reply to: 81197219 [](ancestors = 81197219)

jianchun · 2016-09-29T18:39:43Z

Maybe I can search for all _WIN64 usage -- likely they should all be TARGET_64?

To avoid more noise in this PR, I'll follow up afterwards. We already have #963.

rajatd · 2016-09-29T22:35:09Z

+
+inline Js::Var* _get_va(void* addrOfReturnAddress, int n)
+{
+    Js::Var* pArgs = reinterpret_cast<Js::Var*>(addrOfReturnAddress) + 1;


1 [](start = 71, length = 1)

A comment about what this '1' is for, would be good. (saved frame pointer, right?)

No, RA (ReturnAddress). Args are after RA.

Oh right, my bad.

In reply to: 81249310 [](ancestors = 81249310)

Will add a comment, thanks!

rajatd · 2016-09-30T00:11:39Z

+
+inline int _count_args(Js::CallInfo callInfo)
+{
+    return 2;  // for typical JsMethod with 2 known args "function, callInfo"


Is it possible to somehow enforce/assert here that the other argument is a JavascriptFunction?

This is to support the majority usages of ARGUMENTS(args, callInfo). I can't think of a good way to check that. If it is used incorrectly runtime likely to fail. Hope tests can guard that.

rajatd · 2016-09-30T00:19:14Z

Reviewed most of the backend changes and left relevant comments. Haven't reviewed PDataManager, PrologEncoder, EhFrame and XDataAllocator as I dont know much about that area. Maybe @curtisman @pleath or @LouisLaf can help with that.

curtisman · 2016-10-03T17:34:36Z

Primary changes to JIT on Linux. Custom calling convention ------------------------- Calls to native helpers conform to Sys V AMD64 ABI. - Change all code to use correct arg registers. Calls to JsMethod and native helpers that depend on ARGUMENTS/RUNTIME_ARGUMENTS use custom calling convention: - Caller home arg registers onto stack. Implemented all needed assembly thunks. Unwind data ----------- Emit .eh_frame data for dynamic interpreter thunks and all jit functions. - For less noise, the new code hides behind existing PrologEncoder/PData/XData. Misc ---- ARGUMENTS/RUNTIME_ARGUMENTS: - Extended to support all runtime usages. - Limitation: known arg list must end with "callInfo". Int32Math: - Use builtins to test signed int overflow. Original code does not work due to clang optimization. TODO - AsmJs: missing ArrayBuffer out of bound access recovery. - Simd.

Noisy changes to compile native code gen code on Linux with clang. Most changes are: - Fix missing parentheses around `&&` within `||` expressions - Fix `char16/_u` issues - Fix incorrect `inline`s

PAL related changes to build JIT on Linux. - pal.h: bug fixes in BitScan. - Add missing APIs, enable Event.

Build: - Add backend files. - Enable --no-jit option, and as default on OSX (temporarily). Test: - Add `dynapogo` test variant. Change `interpreted` test variant to create dynamic profiles and input to `dynapogo` test variant. To ensure the same profile file is used for these 2 variants, TestLoader (new) assigns each test case a unique test id, and uses the id to generate dynamic profile file name. - Add 'disable_jit` test variant and as default on OSX (temporarily). - Exclude 2 failing tests that need further work.

Address CR comments. Refactored IntMath classes to share common code and use builtins when available. Cleaned up signed integer overflow cases. Added build.sh --sanity=... support to help future diagnostics. Turned off CLANG signed integer overflow optimization with `-fwrapv` switch. Fixed a ICU call bug (found in running Octane/typescript.js). Implemented another AsmJs thunk. (called by Octane/zlib.js). Synced to latest and fix merge errors.

also merge to latest.

Merge pull request #1591 from jianchun:jit_pr Splitted into 4 commits. JIT: enable JIT on Linux Primary changes to JIT on Linux. Custom calling convention ------------------------- Calls to native helpers conform to Sys V AMD64 ABI. - Change all code to use correct arg registers. Calls to JsMethod and native helpers that depend on ARGUMENTS/RUNTIME_ARGUMENTS use custom calling convention: - Caller home arg registers onto stack. Implemented all needed assembly thunks. Unwind data ----------- Emit .eh_frame data for dynamic interpreter thunks and all jit functions. - For less noise, the new code hides behind existing PrologEncoder/PData/XData. Misc ---- ARGUMENTS/RUNTIME_ARGUMENTS: - Extended to support all runtime usages. - Limitation: known arg list must end with "callInfo". Int32Math: - Use builtins to test signed int overflow. Original code does not work due to clang optimization. TODO - AsmJs: missing ArrayBuffer out of bound access recovery. - Simd. JIT: to compile on Linux Noisy changes to compile native code gen code on Linux with clang. Most changes are: - Fix missing parentheses around `&&` within `||` expressions - Fix `char16/_u` issues - Fix incorrect `inline`s JIT: PAL related changes PAL related changes to build JIT on Linux. - pal.h: bug fixes in BitScan. - Add missing APIs, enable Event. JIT: build and test changes Build: - Add backend files. - Enable --no-jit option, and as default on OSX (temporarily). Test: - Add `dynapogo` test variant. Change `interpreted` test variant to create dynamic profiles and input to `dynapogo` test variant. To ensure the same profile file is used for these 2 variants, TestLoader (new) assigns each test case a unique test id, and uses the id to generate dynamic profile file name. - Add 'disable_jit` test variant and as default on OSX (temporarily). - Exclude 2 failing tests that need further work.

jianchun · 2016-10-03T20:14:50Z

Many thanks to @rajatd @obastemur @curtisman for reviewing this huge PR!
@pleath Please let me know if you have comments, I'll address later.

msftclas added the cla-not-required label Sep 16, 2016

This was referenced Sep 17, 2016

xplat: Date/formatting.js fails with jit on #1593

Closed

xplat: ArrayBuffer oob access recovery [JIT] [ASMJS] #1594

Closed

xplat: enable jit on OSX #1595

Closed

jianchun force-pushed the jit_pr branch from 2b8acaf to 1c7f8a2 Compare September 20, 2016 00:21

jianchun changed the title ~~jit: enable jit on Linux~~ JIT: enable jit on Linux Sep 20, 2016

jianchun changed the title ~~JIT: enable jit on Linux~~ JIT: enable JIT on Linux Sep 20, 2016

jianchun force-pushed the jit_pr branch from 1c7f8a2 to b548c9e Compare September 20, 2016 16:02

obastemur reviewed Sep 21, 2016

View reviewed changes

jianchun force-pushed the jit_pr branch 5 times, most recently from 2b8be73 to d326567 Compare September 24, 2016 03:27

jianchun force-pushed the jit_pr branch from d326567 to ded0397 Compare September 26, 2016 17:29

rajatd reviewed Sep 27, 2016

View reviewed changes

rajatd reviewed Sep 28, 2016

View reviewed changes

jianchun force-pushed the jit_pr branch from ded0397 to 783e64d Compare September 29, 2016 18:41

rajatd reviewed Sep 29, 2016

View reviewed changes

rajatd reviewed Sep 30, 2016

View reviewed changes

jianchun force-pushed the jit_pr branch from 783e64d to 39dce94 Compare September 30, 2016 19:07

jianchun force-pushed the jit_pr branch 2 times, most recently from 03de797 to d126342 Compare October 3, 2016 19:29

Jianchun Xu added 6 commits October 3, 2016 12:35

JIT: to compile on Linux

4f93a9d

Noisy changes to compile native code gen code on Linux with clang. Most changes are: - Fix missing parentheses around `&&` within `||` expressions - Fix `char16/_u` issues - Fix incorrect `inline`s

JIT: PAL related changes

5e1aca9

PAL related changes to build JIT on Linux. - pal.h: bug fixes in BitScan. - Add missing APIs, enable Event.

JIT: (xplat) address CR issues

68e819f

also merge to latest.

jianchun force-pushed the jit_pr branch from d126342 to 68e819f Compare October 3, 2016 19:36

chakrabot merged commit 68e819f into chakra-core:master Oct 3, 2016

jianchun deleted the jit_pr branch October 3, 2016 20:15



		#ifndef __APPLE__
		// BailOutRecord::BailOut(BailOutRecord const * bailOutRecord)

Uh oh!

Conversation

jianchun commented Sep 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jianchun commented Sep 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

obastemur Sep 21, 2016 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jianchun commented Sep 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jianchun commented Sep 16, 2016 •

edited

Loading

jianchun commented Sep 16, 2016 •

edited

Loading

obastemur Sep 21, 2016 •

edited

Loading