[webkit-reviews] review granted: [Bug 70706] Add boolean speculations to DFG JIT 32_64 : [Attachment 112166] the patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Yuqiang Xian
<yuqiang.xian at intel.com>'s request for review:
Bug 70706: Add boolean speculations to DFG JIT 32_64
https://bugs.webkit.org/show_bug.cgi?id=70706

Attachment 112166: the patch
https://bugs.webkit.org/attachment.cgi?id=112166&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=112166&action=review


r=me.  The inline comments are suggestions only.

> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:-952
> -	       m_assembler.testb_i8r(mask.m_value, reg);

Could you have changed the if statement to test both the mask being 0x7f and
the register being one of [ax, bx, cx, dx]?

> Source/JavaScriptCore/dfg/DFGGenerationInfo.h:132
>      switch (from) {

As a side-note, needDataFormatConversion is no longer called from anywhere. 
Your call on whether you want to remove this method in this patch, or not
commit any changes to it, or commit the changes you made.  Your changes to this
method seem to be more-or-less in the spirit of what the method was initially
doing, except that it's not doing anything, since it's not used.

> Source/JavaScriptCore/dfg/DFGJITCompiler32_64.cpp:237
> +		   store32(TrustedImm32(JSValue::BooleanTag),
reinterpret_cast<char*>(scratchBuffer + scratchIndex) +
OBJECT_OFFSETOF(EncodedValueDescriptor, asBits.tag));

The code appears to be placing tags into the OSR scratch buffer, and then
reading pairs of registers from the scratch buffer and placing them into the
register file.	The scratch buffer is only used for temporary storage while
this code is running.  Would it not be more profitable to dump unboxed values
to the scratch buffer, and then box them when transfering from the scratch
buffer back to the register file?  Seems like it would save you a load and a
store, per unboxed virtual register.