Bug 101740

this is related to the MIPS one: https://bugs.webkit.org/show_bug.cgi?id=101704 and the real culprit is this I think: http://trac.webkit.org/changeset/133953

Created attachment 173294 [details] proposed fix.

Comment on attachment 173294 [details] proposed fix. View in context: https://bugs.webkit.org/attachment.cgi?id=173294&action=review > Source/JavaScriptCore/jit/JITPropertyAccess.cpp:355 > -template<IndexingType indexingShape> > -JIT::JumpList JIT::emitGenericContiguousPutByVal(Instruction* currentInstruction, PatchableJump& badType) > +JIT::JumpList JIT::emitGenericContiguousPutByVal(Instruction* currentInstruction, PatchableJump& badType, IndexingType indexingShape) I think the goal if it being a template is to generate a faster implementation, i.e. indexingShape is a compile time constant instead of a variable at run-time that's on the stack. So I don't think it should become a regular method.

(In reply to comment #3) > (From update of attachment 173294 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173294&action=review > > > Source/JavaScriptCore/jit/JITPropertyAccess.cpp:355 > > -template<IndexingType indexingShape> > > -JIT::JumpList JIT::emitGenericContiguousPutByVal(Instruction* currentInstruction, PatchableJump& badType) > > +JIT::JumpList JIT::emitGenericContiguousPutByVal(Instruction* currentInstruction, PatchableJump& badType, IndexingType indexingShape) > > I think the goal if it being a template is to generate a faster implementation, i.e. indexingShape is a compile time constant instead of a variable at run-time that's on the stack. > > So I don't think it should become a regular method. I know the reason, but: """Because templates are compiled when required, this forces a restriction for multi-file projects: the implementation (definition) of a template class or function must be in the same file as its declaration. That means that we cannot separate the interface in a separate header file, and that we must include both interface and implementation in any file that uses the templates.""" and also the GetByVal counterpart is implemented almost in the same way (and not with templates).

(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 173294 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=173294&action=review > > > > > Source/JavaScriptCore/jit/JITPropertyAccess.cpp:355 > > > -template<IndexingType indexingShape> > > > -JIT::JumpList JIT::emitGenericContiguousPutByVal(Instruction* currentInstruction, PatchableJump& badType) > > > +JIT::JumpList JIT::emitGenericContiguousPutByVal(Instruction* currentInstruction, PatchableJump& badType, IndexingType indexingShape) > > > > I think the goal if it being a template is to generate a faster implementation, i.e. indexingShape is a compile time constant instead of a variable at run-time that's on the stack. > > > > So I don't think it should become a regular method. > > I know the reason, but: > """Because templates are compiled when required, this forces a restriction for multi-file projects: the implementation (definition) of a template class or function must be in the same file as its declaration. That means that we cannot separate the interface in a separate header file, and that we must include both interface and implementation in any file that uses the templates.""" > > and also the GetByVal counterpart is implemented almost in the same way (and not with templates). Hmm, isn't the template function called from JITPropertyAccess.cpp and also defined there?

(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > (From update of attachment 173294 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=173294&action=review > > > > > > > Source/JavaScriptCore/jit/JITPropertyAccess.cpp:355 > > > > -template<IndexingType indexingShape> > > > > -JIT::JumpList JIT::emitGenericContiguousPutByVal(Instruction* currentInstruction, PatchableJump& badType) > > > > +JIT::JumpList JIT::emitGenericContiguousPutByVal(Instruction* currentInstruction, PatchableJump& badType, IndexingType indexingShape) > > > > > > I think the goal if it being a template is to generate a faster implementation, i.e. indexingShape is a compile time constant instead of a variable at run-time that's on the stack. > > > > > > So I don't think it should become a regular method. > > > > I know the reason, but: > > """Because templates are compiled when required, this forces a restriction for multi-file projects: the implementation (definition) of a template class or function must be in the same file as its declaration. That means that we cannot separate the interface in a separate header file, and that we must include both interface and implementation in any file that uses the templates.""" > > > > and also the GetByVal counterpart is implemented almost in the same way (and not with templates). > > Hmm, isn't the template function called from JITPropertyAccess.cpp and also defined there? The implementation of emitGenericContiguousPutByVal is in the JITPropertyAccess.cpp/JITPropertyAccess64_32.cpp but it is used in the JIT.h. Maybe this leads to a problem when it is included from somewhere else.

hmmm... maybe if I move the implementation of the emit{Int32,Double,..}PutByVal to the corresponding cpp it would be ok with the templates, but then it should be in both cpps.

(In reply to comment #7) > hmmm... maybe if I move the implementation of the emit{Int32,Double,..}PutByVal to the corresponding cpp it would be ok with the templates, but then it should be in both cpps. I tried moving the implementation of these methods to the general parot of the JITPropertyAccess.cpp file, but The error message remained. After thinking it through the same situation occured. So I think there is only 2 possible solution: 1) use the my first patch which removes the template or 2) move the emit{Int32,..}PutByVal implementation to both JITPropertyAccess files and there will be code duplication.

(In reply to comment #8) > (In reply to comment #7) > > hmmm... maybe if I move the implementation of the emit{Int32,Double,..}PutByVal to the corresponding cpp it would be ok with the templates, but then it should be in both cpps. > > I tried moving the implementation of these methods to the general parot of the JITPropertyAccess.cpp file, but The error message remained. After thinking it through the same situation occured. > > So I think there is only 2 possible solution: > 1) use the my first patch which removes the template or > 2) move the emit{Int32,..}PutByVal implementation to both JITPropertyAccess files and there will be code duplication. The problem is that on mips VALUE_PROFILER not enabled currently and by: inline JITArrayMode JIT::chooseArrayMode(ArrayProfile* profile) { #if ENABLE(VALUE_PROFILER) profile->computeUpdatedPrediction(m_codeBlock); ArrayModes arrayModes = profile->observedArrayModes(); if (arrayProfileSaw(arrayModes, DoubleShape)) return JITDouble; if (arrayProfileSaw(arrayModes, Int32Shape)) return JITInt32; if (arrayProfileSaw(arrayModes, ArrayStorageShape)) return JITArrayStorage; return JITContiguous; #else UNUSED_PARAM(profile); return JITContiguous; #endif } And in jit/JITPropertyAccess32_64.cpp the body of the template function emitGenericContiguousPutByVal<> not instantiated/compiled in for Int32 and Double types. but they are declared in the jit.h header so JITPropertyAccess.cpp can reference to them. I don't know where to put the implementation of template function. Maybe in the header? Or just don't use shorthands like emitInt32PutByVal but template calls as emitGenericContiguousPutByVal<Int32Shape>?

Created attachment 173323 [details] Keep template fix Template implementation moved from JITPropertyAccess32_64.cpp to JITPropertyAccess.cpp.

(In reply to comment #8) > (In reply to comment #7) > > hmmm... maybe if I move the implementation of the emit{Int32,Double,..}PutByVal to the corresponding cpp it would be ok with the templates, but then it should be in both cpps. > > I tried moving the implementation of these methods to the general parot of the JITPropertyAccess.cpp file, but The error message remained. After thinking it through the same situation occured. > > So I think there is only 2 possible solution: > 1) use the my first patch which removes the template or > 2) move the emit{Int32,..}PutByVal implementation to both JITPropertyAccess files and there will be code duplication. In JITPropertyAccess.cpp there is a emitGenericContiguousPutByVal implementation for JSVALUE64 case. When I move the 32 bit implementation of the template function from JITPropertyAccess32_64.cpp to JITPropertyAccess.cpp then the Int32 and Double typed version of the template won't be optimized out and tests are successful with the compiled jsc.

ping?

ping again?

*** Bug 101704 has been marked as a duplicate of this bug. ***

ping?

(In reply to comment #10) > Created an attachment (id=173323) [details] > Keep template fix > > Template implementation moved from JITPropertyAccess32_64.cpp to JITPropertyAccess.cpp. I don't really like this, because you move a JSVALUE32_64 specific code to the general JSPropertyAccess.cpp. A developer could miss it if he/she only checks the 32_64 file. (I know it's not a big reason, but then why does the 32_64.cpp file exists...) If this is not a solid reason, then ok. go with it.

The JSVALUE64 implementation of emitGenericContiguousPutByVal is in JITPropertyAccess.cpp now. It isn't so nice, but I don't have better idea.

Comment on attachment 173323 [details] Keep template fix View in context: https://bugs.webkit.org/attachment.cgi?id=173323&action=review I think it isn't the best way to fix this bug ... but now I don't have better idea. And it is a pending bug fix a valid _build_fail_ after 5 days. I don't think if we should wait more 5 days to fix the build, so r=me with the typo fix I mentioned. > Source/JavaScriptCore/ChangeLog:8 > + Templete function body moved to fix VALUE_PROFILER disabled case. s/Templete/Template

Fix landed in http://trac.webkit.org/changeset/134599 with the changelog fix and removed ugly whitespace lines.

Re-opened since this is blocked by bug 102225

Rolled out by http://trac.webkit.org/changeset/134602, because it broke the 32 bit EFL build: Linking CXX executable ../../../bin/jsc ../../../lib/libjavascriptcore_efl.so.0.1.0: undefined reference to `JSC::AbstractMacroAssembler<JSC::X86Assembler>::JumpList JSC::JIT::emitGenericContiguousPutByVal<(unsigned char)26>(JSC::Instruction*, JSC::AbstractMacroAssembler<JSC::X86Assembler>::PatchableJump&)' ../../../lib/libjavascriptcore_efl.so.0.1.0: undefined reference to `JSC::AbstractMacroAssembler<JSC::X86Assembler>::JumpList JSC::JIT::emitGenericContiguousPutByVal<(unsigned char)20>(JSC::Instruction*, JSC::AbstractMacroAssembler<JSC::X86Assembler>::PatchableJump&)' collect2: ld returned 1 exit status make[2]: *** [bin/jsc] Error 1

Any idea?

(In reply to comment #22) > Any idea? use my patch? :)

(In reply to comment #23) > (In reply to comment #22) > > Any idea? > > use my patch? :) (In reply to comment #22) > Any idea? Could you add any of the patches to commit queue or should I resend/rebase mine? Other patch is ok for me, too just let's go on somehow.

We tried your patch landing ... but we had to revert, because it broke the 32 bit EFL build ... :-/ That's why I asked: Any idea?

Comment on attachment 173294 [details] proposed fix. Let's try this one. :) Working build is more important than performance.

Comment on attachment 173294 [details] proposed fix. Clearing flags on attachment: 173294 Committed r134612: <http://trac.webkit.org/changeset/134612>

All reviewed patches have been landed. Closing bug.

(In reply to comment #22) > Any idea? It's a very hacky solution but if with keeping templates much performance gain can be achieved then: By the error messages the problem comes up when the body of the 3 inline functions from the JIT.h header should be linked. We could force the compiler to compile in the template codes instantiated with all the 3 type parameters with a simple function in JITPropertyAccess32_64.cpp: void forceTemplateCompilation() { emitInt32PutByVal(currentInstruction, badType); // emitGenericContiguousPutByVal<(unsigned char)20> emitDoublePutByVal(currentInstruction, badType); emitContiguousPutByVal(currentInstruction, badType); } This function shouldn't be called ever but as an exportable one the compiler cannot optimize out the 3 version of the template whichever ENABLE_/USE_ macro combination are used. This function would be "Objective-C like private: not mentioned in the header). As I tested the compilation the problems come from that that the above listed 3 functions declared in JIT.h so for <(unsigned char)20>, <(unsigned char)22>, <(unsigned char)26> the template codes of emitGenericContiguousPutByVal<> should be in the object file compiled from the cpp file which contains the template body. But with some ENABLE_ macro configurations the compiler sees that some of the 3 functions not used in JITPropertyAccess32_64.cpp so not compiled in. I can't test it in EFL build env.