-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Jit threading #273
Jit threading #273
Conversation
#if defined(WALRUS_32) | ||
if (!(info & Instruction::kIs32Bit)) { | ||
info = Instruction::kIsCallback; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only I64AtomicLoadOpcode
needs a callback. The rest stores <= 32 bit, so they are supported on 32 bit systems.
src/jit/ByteCodeParser.cpp
Outdated
|
||
Instruction* instr = compiler->append(byteCode, group, opcode, 2, 0); | ||
#if defined(WALRUS_32) | ||
if (!(info & Instruction::kIs32Bit)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
src/jit/MemoryInl.h
Outdated
}; | ||
|
||
MemAddress(uint8_t baseReg, uint8_t offsetReg, uint8_t sourceReg) | ||
: options(0) | ||
MemAddress(uint8_t baseReg, uint8_t offsetReg, uint8_t sourceReg, uint32_t options = 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Options should be the first argument.
src/jit/MemoryInl.h
Outdated
if (size > 1 && (options & CheckNaturalAlignment) && (memArg.argw & (size - 1)) != 0) { | ||
context->appendTrapJump(ExecutionContext::UnalignedAtomicError, sljit_emit_jump(compiler, SLJIT_NOT_EQUAL)); | ||
} | ||
#endif /* ENABLE_EXTENDED_FEATURES */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be done after if (UNLIKELY(offset > context->maximumMemorySize - size)) {
, and emit no instruction if there is an error. Maybe the 'if' part can be combined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How could the two if
s be combined? the alignment check is guarded with ENABLE_EXTENDED_FEATURES
while the other if
is always included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that is ugly. Then duplicate the if
body. I hope the compiler notices that.
src/jit/MemoryInl.h
Outdated
@@ -173,6 +182,16 @@ void MemAddress::check(sljit_compiler* compiler, Operand* offsetOperand, sljit_u | |||
memArg.arg = SLJIT_MEM1(baseReg); | |||
} | |||
#endif /* SLJIT_32BIT_ARCHITECTURE */ | |||
|
|||
#if defined(ENABLE_EXTENDED_FEATURES) | |||
if (size > 1 && (options & CheckNaturalAlignment)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should not set CheckNaturalAlignment
when size == 1
. An ASSERT can be added at the beginning of check.
case ByteCode::I32Store16Opcode: | ||
opcode = SLJIT_MOV32_U16; | ||
size = 2; | ||
break; | ||
#if defined(ENABLE_EXTENDED_FEATURES) | ||
case ByteCode::I64AtomicStoreOpcode: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a special case on 32 bit (only). Calling a helper could be good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case was already done via callback, I have now guarded it, so it does not get included in 32 bit mode.
src/jit/MemoryInl.h
Outdated
@@ -923,6 +991,186 @@ static void emitMemory(sljit_compiler* compiler, Instruction* instr) | |||
} | |||
} | |||
|
|||
#if defined(ENABLE_EXTENDED_FEATURES) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this code at this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure which part of it you mean;
The callbacks are used already on 32 bit, and the switches set the parameters for checking the memory and deciding between native load-store / callback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The case ByteCode::I64AtomicLoad8UOpcode:
is handled by emitLoad, and most other as well. This code also has a , case ByteCode::I64AtomicLoad8UOpcode:
and that is not correct. The only one needs an emulation is 64 bit load/store on 32 bit.
d21fdad
to
5d798ae
Compare
src/jit/Backend.cpp
Outdated
@@ -1237,6 +1237,12 @@ void JITCompiler::compileFunction(JITFunction* jitFunc, bool isExternal) | |||
emitStackInit(m_compiler, item->asInstruction()); | |||
break; | |||
} | |||
#if defined(ENABLE_EXTENDED_FEATURES) | |||
case Instruction::Atomic: { | |||
emitAtomic(m_compiler, item->asInstruction()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this at this point?
src/jit/MemoryInl.h
Outdated
#if defined(ENABLE_EXTENDED_FEATURES) | ||
if (options & CheckNaturalAlignment) { | ||
if (SLJIT_IS_MEM2(memArg.arg)) { | ||
sljit_emit_op2(compiler, SLJIT_ADD, TMP_REG1, 0, baseReg, 0, offsetReg, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does TMP_REG1 comes from? If you compute it, you could also overwrite memArg.arg to SLJIT_MEM1
src/jit/MemoryInl.h
Outdated
sljit_emit_op2(compiler, SLJIT_ADD, TMP_REG1, 0, baseReg, 0, offsetReg, 0); | ||
} | ||
sljit_emit_op2u(compiler, SLJIT_AND | SLJIT_SET_Z, (SLJIT_IS_MEM2(memArg.arg) ? TMP_REG1 : baseReg), 0, SLJIT_IMM, size - 1); | ||
context->appendTrapJump(ExecutionContext::UnalignedAtomicError, sljit_emit_jump(compiler, SLJIT_NOT_EQUAL)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SLJIT_NOT_ZERO
sounds better
@@ -233,6 +263,9 @@ static void emitLoad(sljit_compiler* compiler, Instruction* instr) | |||
opcode = (instr->info() & Instruction::kHasFloatOperand) ? SLJIT_MOV_F64 : SLJIT_MOV; | |||
size = 8; | |||
break; | |||
#if defined(ENABLE_EXTENDED_FEATURES) | |||
case ByteCode::I32AtomicLoadOpcode: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should set CheckNaturalAlignment
.
I would create an options = 0
variable at the beginning of the function and overwrite it in these cases.
src/jit/ByteCodeParser.cpp
Outdated
group = Instruction::Atomic; | ||
|
||
Instruction* instr = compiler->append(byteCode, group, opcode, 1, 1); | ||
#if defined(WALRUS_32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SLJIT has a define for 32/64 bit cpus, it is used in this code.
src/jit/MemoryInl.h
Outdated
sljit_s32 faddr = GET_FUNC_ADDR(sljit_sw, atomicRmwLoad64); | ||
|
||
sljit_emit_op1(compiler, SLJIT_MOV, SLJIT_R0, 0, addr.baseReg, 0); | ||
sljit_emit_op1(compiler, SLJIT_MOV, SLJIT_R1, 0, SLJIT_IMM, size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
size? it should always be 64 bit
src/jit/MemoryInl.h
Outdated
sljit_emit_op1(compiler, SLJIT_MOV, SLJIT_R1, 0, SLJIT_IMM, size); | ||
sljit_emit_icall(compiler, SLJIT_CALL, type, SLJIT_IMM, faddr); | ||
sljit_emit_op1(compiler, SLJIT_MOV, dst.arg2, dst.arg2w, SLJIT_R1, 0); | ||
sljit_emit_op1(compiler, SLJIT_MOV, dst.arg1, dst.arg1w, SLJIT_R0, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not every cpu do things this way.
c9b79ff
to
95abf1d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the callback part is not correct
src/jit/MemoryInl.h
Outdated
@@ -215,14 +246,26 @@ void MemAddress::load(sljit_compiler* compiler) | |||
#endif /* HAS_SIMD */ | |||
} | |||
|
|||
#if defined(ENABLE_EXTENDED_FEATURES) && (defined SLJIT_32BIT_ARCHITECTURE && SLJIT_32BIT_ARCHITECTURE) | |||
static void atomicRmwLoad64(std::atomic<int64_t>& shared, uint64_t* result, int32_t size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a size here? Only 64 bit loads needs callbacks.
src/jit/MemoryInl.h
Outdated
@@ -439,6 +539,24 @@ static void emitLoad(sljit_compiler* compiler, Instruction* instr) | |||
#endif /* HAS_SIMD */ | |||
|
|||
#if (defined SLJIT_32BIT_ARCHITECTURE && SLJIT_32BIT_ARCHITECTURE) | |||
#if defined(ENABLE_EXTENDED_FEATURES) | |||
if (isAtomic && opcode == SLJIT_MOV) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you add a helper function, which is called directly after I64AtomicLoadOpcode
, then you don't need isAtomic boolean.
src/jit/MemoryInl.h
Outdated
sljit_s32 type = SLJIT_ARGS3V(P, P, W); | ||
sljit_s32 faddr = GET_FUNC_ADDR(sljit_sw, atomicRmwLoad64); | ||
|
||
sljit_emit_op1(compiler, SLJIT_MOV, SLJIT_R0, 0, addr.baseReg, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to use addr.memArg
similar to other cases.
95abf1d
to
021933a
Compare
src/jit/MemoryInl.h
Outdated
@@ -358,6 +439,53 @@ static void emitLoad(sljit_compiler* compiler, Instruction* instr) | |||
size = 8; | |||
break; | |||
#endif /* HAS_SIMD */ | |||
#if defined(ENABLE_EXTENDED_FEATURES) | |||
case ByteCode::I32AtomicLoadOpcode: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original code reused the regular opcodes. The only thing needs to be added a options |= MemAddress::CheckNaturalAlignment;
and a fallthrough. Is it better this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this way is less visually busy, but I can change it back if you wish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could introduce a macro, which does nothing, if the extension is not defined. Otherwise it defines the necessary lines. This way each new opcode requires one line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed it back; I feel a macro would have to be very complex to handle the different cases of the CheckNaturalAlignment flag not always being there, and the callbacks for the various sizes and load/store variants.
When I originally changed it there were more things there, that are no longer there, so on a second look its not too busy.
src/jit/MemoryInl.h
Outdated
*result = shared.load(std::memory_order_relaxed); | ||
} | ||
|
||
static void emitAtomicLoad64(sljit_compiler* compiler, Instruction* instr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we combine this with store to reduce code size?
src/jit/MemoryInl.h
Outdated
@@ -215,11 +246,61 @@ void MemAddress::load(sljit_compiler* compiler) | |||
#endif /* HAS_SIMD */ | |||
} | |||
|
|||
#if defined(ENABLE_EXTENDED_FEATURES) && (defined SLJIT_32BIT_ARCHITECTURE && SLJIT_32BIT_ARCHITECTURE) | |||
static void atomicRmwLoad64(std::atomic<int64_t>& shared, uint64_t* result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this valid to pas a pointer as std::atomic<int64_t>&
? For me it would be safer to pass the pointer, and convert it to this special type.
3ad4f2c
to
cef8b1a
Compare
src/jit/MemoryInl.h
Outdated
MemoryStore* storeOperation = reinterpret_cast<MemoryStore*>(instr->byteCode()); | ||
offset = storeOperation->offset(); | ||
} else { | ||
ASSERT_NOT_REACHED(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are too long for me. Just use } else {
and add ASSERT(instr->opcode() == ByteCode::I64AtomicStoreOpcode);
cef8b1a
to
81ea9df
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patch should be converted to ready for review.
src/jit/MemoryInl.h
Outdated
faddr = GET_FUNC_ADDR(sljit_sw, atomicRmwStore64); | ||
} | ||
|
||
if (valueArgPair.arg1 != SLJIT_MEM1(kFrameReg)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be done only for stores, am I right? Could be moved into the else above.
81ea9df
to
a0cfd72
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
If you update/complete JIT thread operations in the following patches, please verify it by running the test cases (TCs) located in test/extended
Thank you. We are testing it, although tests are not fully running. |
Initial version, contains Loads and Stores only (no RMW, wait, notify, or fence).