Bug 160588 – Refactor MathIC compilation process in Baseline and DFG to turn temporary registers usage more flexible

• Component: JavaScriptCore
• Status: NEW

[Caio Lima]

Refactor MathIC compilation in LowerDFGToB3::compileMathIC and JIT::emitMathICFast to make temporary registers usage more flexible. In current implementation is it possible use one scratchGPR and one scratchFPR.

[Caio Lima]

Created attachment 288148 [details]
WIP refactoring MathIC scratch rules

Tests are ok in x86_64 but lets see what EWS thinks about it.

[Caio Lima]

Created attachment 288391 [details]
Patch

[Caio Lima]

ping review

[Saam Barati]

Comment on attachment 288391 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288391&action=review

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3457
> +    for (int i = 0; i < MATH_IC_MAX_GPR; i++)

should be uint32_t or size_t. (For all loops below, too.)

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3482
> +    if (scratchGPRRegCount == 1)

Hmm, this doesn't like it'd scale well.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1583
> +        patchpoint->numGPScratchRegisters = numGPScratchRegisters;
> +        patchpoint->numFPScratchRegisters = 2 + numFPScratchRegisters;

The idea of doing this is so that we don't require the 2 extra FP registers when not needed. So the caller should be responsible of providing these numbers.

[Caio Lima]

Comment on attachment 288391 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288391&action=review

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3457
>> +    for (int i = 0; i < MATH_IC_MAX_GPR; i++)
> 
> should be uint32_t or size_t. (For all loops below, too.)

Done.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3482
>> +    if (scratchGPRRegCount == 1)
> 
> Hmm, this doesn't like it'd scale well.

I agree, however I am getting the following error because there is no sufficient register:

sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: ASSERTION FAILED: currentLowest != NUM_REGS && currentSpillOrder != SpillHintInvalid
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: /Users/caiolima/open_projects/WebKit/Source/JavaScriptCore/dfg/DFGRegisterBank.h(138) : RegID JSC::DFG::RegisterBank<JSC::GPRInfo>::allocate(JSC::VirtualRegister &) [BankInfo = JSC::GPRInfo]
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 1   0xe96ffa WTFCrash
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 2   0x6cf606 JSC::DFG::RegisterBank<JSC::GPRInfo>::allocate(JSC::VirtualRegister&)
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 3   0x6b45f5 JSC::DFG::SpeculativeJIT::allocate()
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 4   0x6fe7cd JSC::DFG::SpeculativeJIT::fillJSValue(JSC::DFG::Edge, JSC::X86Registers::RegisterID&, JSC::X86Registers::RegisterID&, JSC::X86Registers::XMMRegisterID&)
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 5   0x6d18c2 JSC::DFG::JSValueOperand::fill()
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 6   0x6b9c35 JSC::DFG::JSValueOperand::tagGPR()
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 7   0x6b4cda JSC::DFG::JSValueOperand::jsValueRegs()
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 8   0x6c4764 void JSC::DFG::SpeculativeJIT::compileMathIC<JSC::JITMulGenerator, long long (*)(JSC::ExecState*, long long, long long, JSC::JITMathIC<JSC::JITMulGenerator>*), long long (*)(JSC::ExecState*, long long, long long)>(JSC::DFG::Node*, JSC::JITMathIC<JSC::JITMulGenerator>*, unsigned char, unsigned char, long long (*)(JSC::ExecState*, long long, long long, JSC::JITMathIC<JSC::JITMulGenerator>*), long long (*)(JSC::ExecState*, long long, long long))
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 9   0x6926f2 JSC::DFG::SpeculativeJIT::compileArithMul(JSC::DFG::Node*)
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 10  0x70ea82 JSC::DFG::SpeculativeJIT::compile(JSC::DFG::Node*)
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 11  0x68130f JSC::DFG::SpeculativeJIT::compileCurrentBlock()
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 12  0x681c9f JSC::DFG::SpeculativeJIT::compile()
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 13  0x5264bc JSC::DFG::JITCompiler::compileBody()
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 14  0x52a08a JSC::DFG::JITCompiler::compileFunction()
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 15  0x631f70 JSC::DFG::Plan::compileInThreadImpl(JSC::DFG::LongLivedState&)
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 16  0x630a8a JSC::DFG::Plan::compileInThread(JSC::DFG::LongLivedState&, JSC::DFG::ThreadData*)
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 17  0x4a6280 JSC::DFG::compileImpl(JSC::VM&, JSC::CodeBlock*, JSC::CodeBlock*, JSC::DFG::CompilationMode, unsigned int, JSC::Operands<JSC::JSValue> const&, WTF::PassRefPtr<JSC::DeferredCompilationCallback>)
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 18  0x4a5c62 JSC::DFG::compile(JSC::VM&, JSC::CodeBlock*, JSC::CodeBlock*, JSC::DFG::CompilationMode, unsigned int, JSC::Operands<JSC::JSValue> const&, WTF::PassRefPtr<JSC::DeferredCompilationCallback>)
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 19  0x973336 operationOptimize
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 20  0x2b69d27
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 21  0xb63964 llint_entry
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 22  0xb639bb llint_entry
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 23  0x2b5f118
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 24  0xb639bb llint_entry
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 25  0xb5e41c vmEntryToJavaScript
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 26  0x95888f JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 27  0x8f5943 JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*)
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 28  0x2babb5 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 29  0x54ae8 runWithScripts(GlobalObject*, WTF::Vector<Script, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::String const&, bool, bool, bool)
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 30  0x53c14 runJSC(JSC::VM*, CommandLine)
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: 31  0x52a69 jscmain(int, char**)
sunspider-1.0/3d-raytrace.js.no-cjit-validate-phases: test_script_19: line 2: 78419 Segmentation fault: 11  ( "$@" ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --useFunctionDotArguments\=true --maxPerThreadStackUsage\=1572864 --validateBytecode\=true --validateGraphAtEachPhase\=true --useSourceProviderCache\=false --useConcurrentJIT\=false --thresholdForJITAfterWarmUp\=100 --scribbleFreeCells\=true 3d-raytrace.js )

One way I am thinking now is ASSERT(MATH_IC_MAX_GPR <= 2) for x86_32 cases, However it still doesn't solve our limitation on https://bugs.webkit.org/show_bug.cgi?id=160012 because we need 3 scratchGPRs there. Do you think spill the registers makes sense here?

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1583
>> +        patchpoint->numFPScratchRegisters = 2 + numFPScratchRegisters;
> 
> The idea of doing this is so that we don't require the 2 extra FP registers when not needed. So the caller should be responsible of providing these numbers.

Done.

[Caio Lima]

Created attachment 289528 [details]
Patch

[WebKit Commit Bot]

Attachment 289528 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3506:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3509:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3511:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3518:  Use std::min() or std::min<type>() instead of the MIN() macro.  [runtime/max_min_macros] [4]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Caio Lima]

Since we have asserts on JIT*Generators that scratchGPR != (left/right).tagGPR, I didn't change the Generators because it is expected to allocate just 1 scratchGPR on current MathGenerators. I added the case to use left/right tagGPRs on 32-bits, because there are some potentials Generators that can use them in the future (it is the case of JITPowGenerator that is being developed on https://bugs.webkit.org/show_bug.cgi?id=160012).

[Caio Lima]

Created attachment 289532 [details]
Patch

[Build Bot]

Comment on attachment 289532 [details]
Patch

Attachment 289532 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2123955

New failing tests:
fast/canvas/webgl/premultiplyalpha-test.html
fast/canvas/canvas-longlived-context.html
fast/forms/input-maxlength.html
fast/workers/dedicated-worker-lifecycle.html
js/for-in-modify-in-loop.html
fast/forms/input-implicit-length-limit.html

[Build Bot]

Created attachment 289538 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289532 [details]
Patch

Attachment 289532 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2124024

New failing tests:
imported/w3c/web-platform-tests/html/dom/reflection-grouping.html
fast/canvas/webgl/premultiplyalpha-test.html
imported/w3c/web-platform-tests/html/dom/reflection-text.html
fast/canvas/canvas-longlived-context.html
imported/w3c/web-platform-tests/html/dom/reflection-misc.html
fast/forms/input-maxlength.html
imported/w3c/web-platform-tests/html/dom/reflection-tabular.html
imported/w3c/web-platform-tests/html/dom/reflection-sections.html
fast/frames/take-focus-from-iframe.html
imported/w3c/web-platform-tests/html/dom/reflection-embedded.html
fast/workers/dedicated-worker-lifecycle.html
js/for-in-modify-in-loop.html
fast/canvas/webgl/object-deletion-behaviour.html
fast/forms/input-implicit-length-limit.html
imported/w3c/web-platform-tests/html/dom/reflection-forms.html

[Build Bot]

Created attachment 289540 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289532 [details]
Patch

Attachment 289532 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2124028

New failing tests:
imported/w3c/web-platform-tests/html/dom/reflection-grouping.html
fast/canvas/webgl/premultiplyalpha-test.html
imported/w3c/web-platform-tests/html/dom/reflection-text.html
fast/canvas/canvas-longlived-context.html
imported/w3c/web-platform-tests/html/dom/reflection-misc.html
fast/forms/input-maxlength.html
imported/w3c/web-platform-tests/html/dom/reflection-tabular.html
imported/w3c/web-platform-tests/html/dom/reflection-sections.html
imported/w3c/web-platform-tests/html/dom/reflection-embedded.html
fast/workers/dedicated-worker-lifecycle.html
js/for-in-modify-in-loop.html
fast/forms/input-implicit-length-limit.html
imported/w3c/web-platform-tests/html/dom/reflection-forms.html

[Build Bot]

Created attachment 289541 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Caio Lima]

Created attachment 289565 [details]
Patch

[Saam Barati]

Comment on attachment 289565 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289565&action=review

Mostly LGTM, but I think the patch can become better.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3525
> +    // If there is no more GPR available to allocate, we get tagGPR from
> +    // result, right and left operands. It is possible because they are speculated
> +    // and generators know how to restore the tags after operations using these GPRs.

This seems sketchy and probably a bad API. If the 32-bit implementations already do this, why do they even need us to pass in a scratch? They should just do it and have it be opaque to the thing allocating the scratches.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1559
> +        compileMathIC(addIC, 1, 2, repatchingFunction, nonRepatchingFunction);

Style: Please give these names as local variables and pass in the variables to the function.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1598
> +                for (int i = 0; i < MATH_IC_MAX_GPR; i++) {

Don't use "int"

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1600
> +                        scratchGPRReg[i] = params.gpScratch(0);

This is wrong.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1606
> +                for (int i = 0; i < MATH_IC_MAX_GPR; i++) {

Don't use "int"

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1748
> +            compileMathIC(subIC, 1, 2, repatchingFunction, nonRepatchingFunction);

Ditto w.r.t names as above.
Also, why does this need 2 scratch FPRs, but the DFG version needs 1. They should be consistent. However, reading this code, I think the numberOfGPRScratches/numberOfFPRScratches should just be static fields on the various Generator classes, and not on the creators of the ICs.
This seems like a cleaner approach because each place a MulIC (for example) is created, the creator doesn't have to always specify 1 and 1. The Generator will just know.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1842
> +            compileMathIC(mulIC, 1, 2, repatchingFunction, nonRepatchingFunction);

Ditto w.r.t names and using Generator to dictate how many scratches are needed.

> Source/JavaScriptCore/jit/JITMathIC.h:55
> +#define MATH_IC_MAX_FPR 3

Which IC uses 3?

[Caio Lima]

Comment on attachment 289565 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289565&action=review

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3525
>> +    // and generators know how to restore the tags after operations using these GPRs.
> 
> This seems sketchy and probably a bad API. If the 32-bit implementations already do this, why do they even need us to pass in a scratch? They should just do it and have it be opaque to the thing allocating the scratches.

Nice point. I totally agree with that. The current implementation already reuses the result.tagGPR as scratchGPR. I am going to move it to Generators and remove this allocation here.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1748
>> +            compileMathIC(subIC, 1, 2, repatchingFunction, nonRepatchingFunction);
> 
> Ditto w.r.t names as above.
> Also, why does this need 2 scratch FPRs, but the DFG version needs 1. They should be consistent. However, reading this code, I think the numberOfGPRScratches/numberOfFPRScratches should just be static fields on the various Generator classes, and not on the creators of the ICs.
> This seems like a cleaner approach because each place a MulIC (for example) is created, the creator doesn't have to always specify 1 and 1. The Generator will just know.

Actually, the correct semantic behavior is 0. If you check current implementation "patchpoint->numFPScratchRegisters = 2" because leftFPR and rightFPR are allocated using "patchpoint->numFPScratchRegisters". That was the reason that in my last Patch I was using "patchpoint->numFPScratchRegisters = 2 + numFPScratchRegisters;". I think that is better the way that I implemented before, because it is going to keep numFPScratchRegisters consistent between DFG and FTL layers.

>> Source/JavaScriptCore/jit/JITMathIC.h:55
>> +#define MATH_IC_MAX_FPR 3
> 
> Which IC uses 3?

It is because the MathIC for B3 allocates right, left and scratch FPRs using "patchpoint->numFPScratchRegisters = numFPScratchRegisters". For all generators scratchFPR are always InvalidFPRReg.

[Caio Lima]

Created attachment 289792 [details]
Patch

[Caio Lima]

Created attachment 289795 [details]
Patch

[Caio Lima]

Created attachment 289796 [details]
Patch

[Saam Barati]

Comment on attachment 289796 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289796&action=review

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3434
> +    ASSERT(scratchFPRRegCount <= MATH_IC_MAX_GPR);
> +    ASSERT(scratchFPRRegCount <= MATH_IC_MAX_FPR);

Please make this a static_assert

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3512
> +    // Since registers on 32-bit architectures like x86_32 are scarce
> +    // we need to check if it is possible allocate new registers

It seems like all 32-bit snippets are able to use tags as their scratch, can we just remove this code?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1569
> +        ASSERT(numGPScratchRegisters <= MATH_IC_MAX_GPR);
> +        ASSERT(numFPScratchRegisters <= MATH_IC_MAX_FPR);

Please make this a static_assert

> Source/JavaScriptCore/jit/JITMathIC.h:56
> +#define MATH_IC_MAX_GPR 1
> +#define MATH_IC_MAX_FPR 1

I would make these static fields inside JITMathIC instead of a define.

> Source/JavaScriptCore/jit/JITMulGenerator.cpp:41
> +    if (m_scratchGPR == InvalidGPRReg)
> +        m_scratchGPR = m_result.tagGPR();

Have you made sure it's always valid to do this?

[Caio Lima]

Comment on attachment 289796 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289796&action=review

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3434
>> +    ASSERT(scratchFPRRegCount <= MATH_IC_MAX_FPR);
> 
> Please make this a static_assert

Ok

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3512
>> +    // we need to check if it is possible allocate new registers
> 
> It seems like all 32-bit snippets are able to use tags as their scratch, can we just remove this code?

My intention is to enable allocate scratches and avoid use tagGPRs to avoid instructions to restore the tags. Current Snnipets uses just one scratch that is mapped to result .tagGPR , but others like JITPowGenerator can use more. If scratches are already allocated, we don't need to use x/y.tagGPR and consequently, we don't need to restore their tags on snnipets.

>> Source/JavaScriptCore/jit/JITMathIC.h:56
>> +#define MATH_IC_MAX_FPR 1
> 
> I would make these static fields inside JITMathIC instead of a define.

Good. I am going to do that.

>> Source/JavaScriptCore/jit/JITMulGenerator.cpp:41
>> +        m_scratchGPR = m_result.tagGPR();
> 
> Have you made sure it's always valid to do this?

The current code always  use result.tagGPR as scratch on 32bits. So I suppose that it is valid. Also, AFIK all results assign are boxed, so the tag is going to be restored properly. I can double check that.

[Caio Lima]

Created attachment 290564 [details]
Patch

[Caio Lima]

Created attachment 291111 [details]
Patch

[Saam Barati]

Comment on attachment 291111 [details]
Patch

Something about this patch doesn't sit right with me, and I think I've finally figured out what it is.
This API doesn't seem scalable to adding more math ICs that need more scratch registers. I think we need
to move to a more dynamic API, by either passing in a vector of scratch registers, or a RegisterSet of used registers.
The reason this code feels so weird to me is that you've started to make this API more dynamic, yet you're still calling
the exact same constructors. So it's only pretending to be more dynamic, but in reality, it's not.
Also, the code is just much harder to follow now. 
I'm not sure what the best API is. Here are two options:
1. If we pass in a RegisterSet of used registers, each snippet will now be responsible for allocating scratch
registers using the scratch register allocator.
2. Alternatively, we can keep most of your patch like it is now, but pass in a vector of scratch registers.
Each snippet constructor can deconstruct the vector into member variables and assert that none are InvalidGPR/FPRReg.
The vector can have an inlinecapacity of scratchGPRRegCount/scratchFPRRegCount and just be the vector of registers.

I think I prefer 2. Maybe you have other ideas?

[Caio Lima]

(In reply to comment #26)
> Comment on attachment 291111 [details]
> Patch
> 
> Something about this patch doesn't sit right with me, and I think I've
> finally figured out what it is.
> This API doesn't seem scalable to adding more math ICs that need more
> scratch registers. I think we need
> to move to a more dynamic API, by either passing in a vector of scratch
> registers, or a RegisterSet of used registers.
> The reason this code feels so weird to me is that you've started to make
> this API more dynamic, yet you're still calling
> the exact same constructors. So it's only pretending to be more dynamic, but
> in reality, it's not.
> Also, the code is just much harder to follow now. 
> I'm not sure what the best API is. Here are two options:
> 1. If we pass in a RegisterSet of used registers, each snippet will now be
> responsible for allocating scratch
> registers using the scratch register allocator.
> 2. Alternatively, we can keep most of your patch like it is now, but pass in
> a vector of scratch registers.
> Each snippet constructor can deconstruct the vector into member variables
> and assert that none are InvalidGPR/FPRReg.
> The vector can have an inlinecapacity of
> scratchGPRRegCount/scratchFPRRegCount and just be the vector of registers.
> 
> I think I prefer 2. Maybe you have other ideas?

I remember discussing with you a long time ago that I proposed to you use a vector, however you gave me an answer that convinced me. The problem is that the vector is going to inflate all ICs, mainly the ones that use just 1 scratch.

If I understood your idea, we could use GPRReg allocatedRegs[scratchGPRRegCount] and pass it to JIT*Generator and the Generator just validate if they are invalid or not. This implementation is going to use sizeof(GPRReg * scratchGPRRegCount) memory, which means sizeof(GPRReg) if scratchGPRRegCount = 1. Is it right? If yes, I can't think in a better solution right now and the idea of Generators being responsible to allocate its scratches doesn't sound right IMO.

[Caio Lima]

Created attachment 292502 [details]
Patch

[WebKit Commit Bot]

Attachment 292502 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITMulGenerator.h:53:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITMulGenerator.h:54:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:52:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:53:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:53:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:54:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 6 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Caio Lima]

Created attachment 294030 [details]
Patch

[WebKit Commit Bot]

Attachment 294030 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITMulGenerator.h:53:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITMulGenerator.h:54:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:52:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:53:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:53:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:54:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 6 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 294030 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294030&action=review

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3563
> +    GPRReg scratchGPR[Generator::scratchGPRRegCount];
> +    FPRReg scratchFPR[Generator::scratchFPRRegCount];

I'm more of a fan of using Vector<> with an inline capacity for this API. You'll still get the same property where you don't end up mallocing any memory.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3600
> +    for (size_t i = 0; i < std::min<unsigned>(availableGPRs, scratchGPRRegCount); i++) {

It seems wrong if the min() expression ever returns availableGPRs instead of scratchGPRRegCount. I would just turn this into an assert that we have enough registers.

> Source/JavaScriptCore/jit/JITMulGenerator.h:45
> +        FPRReg leftFPR, FPRReg rightFPR, GPRReg *scratchGPRs, FPRReg *scratchFPRs)

See my comment about using Vector here instead. You could even templatize it over Vector type to not have to worry about what the inline capacity of it is.

[Caio Lima]

Comment on attachment 294030 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294030&action=review

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3563
>> +    FPRReg scratchFPR[Generator::scratchFPRRegCount];
> 
> I'm more of a fan of using Vector<> with an inline capacity for this API. You'll still get the same property where you don't end up mallocing any memory.

Sorry for my ignorance here. I used Vector with inline capacity in another Patch this weekend, however I didn't totally understand it .Do you mind explain me what is the difference of use a Vector<int>, for example?

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3600
>> +    for (size_t i = 0; i < std::min<unsigned>(availableGPRs, scratchGPRRegCount); i++) {
> 
> It seems wrong if the min() expression ever returns availableGPRs instead of scratchGPRRegCount. I would just turn this into an assert that we have enough registers.

hum...We can reproduce it on x86_32, for sure, because old implementation uses result.tagGPR as scratchGPR because of limited registers. I am going to double check it.

[Saam Barati]

(In reply to comment #33)
> Comment on attachment 294030 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294030&action=review
> 
> >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3563
> >> +    FPRReg scratchFPR[Generator::scratchFPRRegCount];
> > 
> > I'm more of a fan of using Vector<> with an inline capacity for this API. You'll still get the same property where you don't end up mallocing any memory.
> 
> Sorry for my ignorance here. I used Vector with inline capacity in another
> Patch this weekend, however I didn't totally understand it .Do you mind
> explain me what is the difference of use a Vector<int>, for example?
Using a Vector with an inline capacity as a stack allocated variable will prevent it from mallocing a backing store until you've exceeded InlineCapacity number of elements. You should read the entire vector class to get a better idea of what I'm saying, but at a high level, it has a field like this:

template <typename T, size_t InlineCapacity>
class Vector {
     T m_inlineVector[InlineCapacity];
}

> 
> >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3600
> >> +    for (size_t i = 0; i < std::min<unsigned>(availableGPRs, scratchGPRRegCount); i++) {
> > 
> > It seems wrong if the min() expression ever returns availableGPRs instead of scratchGPRRegCount. I would just turn this into an assert that we have enough registers.
> 
> hum...We can reproduce it on x86_32, for sure, because old implementation
> uses result.tagGPR as scratchGPR because of limited registers. I am going to
> double check it.

[Caio Lima]

Created attachment 295895 [details]
Patch

[WebKit Commit Bot]

Attachment 295895 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITMulGenerator.h:60:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:59:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:60:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Caio Lima]

Created attachment 297817 [details]
Patch

[Caio Lima]

(In reply to comment #37)
> Created attachment 297817 [details]
> Patch

Rebased with master and created guards for GPR/FPR Scratch Count = 0.

Let's see what EWS thinks about this Patch.

[Build Bot]

Comment on attachment 297817 [details]
Patch

Attachment 297817 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2800668

New failing tests:
svg/in-html/sizing/svg-inline.html
webgl/1.0.2/conformance/uniforms/out-of-bounds-uniform-array-access.html

[Build Bot]

Created attachment 297818 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Caio Lima]

Created attachment 297820 [details]
Patch

[WebKit Commit Bot]

Attachment 297820 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITMulGenerator.h:60:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:59:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:60:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 297820 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297820&action=review

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3572
> +        } else 
> +            scratchGPR[i] = InvalidGPRReg;

Is this side of the branch ever taken? It seems like we should almost assert against it, and if a generator knows how to handle using the tags, it should just *always* do that and report that it needs fewer registers.

> Source/JavaScriptCore/jit/JITAddGenerator.h:52
> +        FPRReg leftFPR, FPRReg rightFPR, Vector<GPRReg, scratchGPRRegCount> scratchGPRs, Vector<FPRReg, scratchFPRRegCount> scratchFPRs)

You shouldn't be copying these vectors for these constructors. I'd use "const Vector<...>&" as the type here.

> Source/JavaScriptCore/jit/JITArithmetic.cpp:777
> +        else
> +            scratchGPR[i] = InvalidGPRReg;

Ditto as the DFG code that does this.

> Source/JavaScriptCore/jit/JITMulGenerator.cpp:41
> +    // Maybe it was not possible allocate scratchGPR. In this case, we use result.tagGPR
> +    if (m_scratchGPR == InvalidGPRReg)
> +        m_scratchGPR = m_result.tagGPR();

Ditto, we should just always use tag if we're 32-bit and we know we need a scratch and that the code is valid when using the tag as a scratch.

> Source/JavaScriptCore/jit/JITMulGenerator.h:52
> +        FPRReg leftFPR, FPRReg rightFPR, Vector<GPRReg, scratchGPRRegCount> scratchGPRs, Vector<FPRReg, scratchFPRRegCount> scratchFPRs)

Ditto about copying the vectors.

> Source/JavaScriptCore/jit/JITSubGenerator.h:51
> +        FPRReg leftFPR, FPRReg rightFPR, Vector<GPRReg, scratchGPRRegCount> scratchGPRs, Vector<FPRReg, scratchFPRRegCount> scratchFPRs)

Ditto about copying the vectors.

[Caio Lima]

Comment on attachment 297820 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297820&action=review

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3572
>> +            scratchGPR[i] = InvalidGPRReg;
> 
> Is this side of the branch ever taken? It seems like we should almost assert against it, and if a generator knows how to handle using the tags, it should just *always* do that and report that it needs fewer registers.

The problem here actually isn't the DG version, but the Baseline version on JITArithmetic.cpp. Take a look at https://github.com/caiolima/webkit/blob/math-ic-scratch/Source/JavaScriptCore/jit/JITArithmetic.cpp#L761
In this case, resultRegs = leftRegs, which makes impossible use resultRegs.tagGPR in that case. This way, we really need this mechanism to make sure that Api works across DFG and Baseline.

[Caio Lima]

View in context: https://bugs.webkit.org/attachment.cgi?id=297820&action=review

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3572
>> +            scratchGPR[i] = InvalidGPRReg;
> 
> Is this side of the branch ever taken? It seems like we should almost assert against it, and if a generator knows how to handle using the tags, it should just *always* do that and report that it needs fewer registers.

The problem here actually isn't the DG version, but the Baseline version on JITArithmetic.cpp. Take a look at https://github.com/caiolima/webkit/blob/math-ic-scratch/Source/JavaScriptCore/jit/JITArithmetic.cpp#L761
In this case, resultRegs = leftRegs, which makes impossible use resultRegs.tagGPR in that case. This way, we really need this mechanism to make sure that Api works across DFG and Baseline.

[Caio Lima]

Created attachment 297922 [details]
Patch

[Caio Lima]

ping review

[zooz hada]

تصحيح

[Michael Catanzaro]

Thanks for bumping this, mysterious spammer. Caio probably still wants a review!

[Maciej Stachowiak]

Comment on attachment 297922 [details]
Patch

I don't have the expertise to review this on a substantive level, but at the very least, this patch needs to be rebaselined as it no longer applies.