Bug 151735

Created attachment 266419 [details] Patch

Comment on attachment 266419 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266419&action=review This looks really good - but I think you should change how you use FPRInfo. > Source/JavaScriptCore/ChangeLog:25 > + information from block to block until no unsafe chain in found. *is found > Source/JavaScriptCore/ChangeLog:34 > + -Anything with a special can have hidden instructions. We should weight that. Interesting! Most specials can tell you precisely what is going on: C call can tell you that it's a call, and checks can tell you the exact instruction. The patchpoint special is super weird though. If we can abstract the effect on the FPU as something as broad as "doesFloatMath" then we could add it as a field in B3::PatchpointValue. > Source/JavaScriptCore/b3/air/AirFixPartialRegisterStalls.cpp:126 > +#if !CPU(X86_64) Let's use "if (!isX86()) return;". I think that it's best if the compiler avoids #if's. I've found it to be super confusing because a compiler usually has two inherent levels of meta: there is the program and there is the program that the program is operating on. The C preprocessor adds a third-level - now you have a program that is modifying the program that is operating on some program. Luckily, the meta nature of a compiler means that you can get away with run-time architecture checks. And the "run-time check" that I'm proposing is actually constant-folded by the compiler anyway. > Source/JavaScriptCore/b3/air/AirFixPartialRegisterStalls.cpp:217 > + if (localDistance.distance[FPRInfo::toIndex(reg.fpr())] < minimumSafeDistance) We should avoid using GPRInfo and FPRInfo register indexing in Air. If we need something like a register index, we should introduce this as a concept in Air or somewhere else. This is because GPRInfo and FPRInfo have register "indexes" for those registers that are legal to use for user values in the DFG. > Source/JavaScriptCore/jit/FPRInfo.h:49 > - static const unsigned numberOfRegisters = 6; > +#if CPU(X86_64) > + static const unsigned numberOfRegisters = 16; > +#else > + static const unsigned numberOfRegisters = 8; > +#endif > + This is a potentially dangerous change, since it makes the DFG use more xmm registers. That's possibly bad because there may be assumptions about the top xmm registers being available for scratch. I think if we wanted to change this constant, then we should do it as its own patch and with the understanding that FPRInfo::numberOfRegisters is NOT the number of FPRs in the CPU, but rather, the number of FPU registers that we want the DFG to use. I don't think you should need to touch this code, because it would be wrong to use GPRInfo/FPRInfo "indexes" inside Air. Those indexes have a pretty specific meaning in the DFG register allocator and the scratch register allocator used in polymorphic inline caches. So, I think that what you really want is a true notion of register index - i.e. not what GPRInfo/FPRInfo call the "index". If you want this, then you should create such a thing, either in the assembler directory - possibly as a new API in MacroAssembler - or as a set of helpers inside Air.

Created attachment 266428 [details] Patch

(In reply to comment #2) > This is a potentially dangerous change, since it makes the DFG use more xmm > registers. That's possibly bad because there may be assumptions about the > top xmm registers being available for scratch. I think if we wanted to > change this constant, then we should do it as its own patch and with the > understanding that FPRInfo::numberOfRegisters is NOT the number of FPRs in > the CPU, but rather, the number of FPU registers that we want the DFG to use. > > I don't think you should need to touch this code, because it would be wrong > to use GPRInfo/FPRInfo "indexes" inside Air. Those indexes have a pretty > specific meaning in the DFG register allocator and the scratch register > allocator used in polymorphic inline caches. > > So, I think that what you really want is a true notion of register index - > i.e. not what GPRInfo/FPRInfo call the "index". If you want this, then you > should create such a thing, either in the assembler directory - possibly as > a new API in MacroAssembler - or as a set of helpers inside Air. Looks like the MacroAssembler already had what we need.

Created attachment 266433 [details] Patch

Created attachment 266448 [details] Patch for landing

Comment on attachment 266448 [details] Patch for landing Clearing flags on attachment: 266448 Committed r192946: <http://trac.webkit.org/changeset/192946>

All reviewed patches have been landed. Closing bug.