Bug 70706 – Add boolean speculations to DFG JIT 32_64

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yuqiang Xian]

Working on boolean speculations in DFG JIT 32_64..

[Yuqiang Xian]

Created attachment 112136 [details]
WIP patch

WIP - this patch adds the DataFormatBoolean support to DFG 32_64 as well as some fixes on SpillOrder and needDataFormatConversion.
The actual speculation part is not included in this patch.

[Yuqiang Xian]

Created attachment 112166 [details]
the patch

The patch for review. Thanks.

[Yuqiang Xian]

(In reply to comment #2)
> Created an attachment (id=112166) [details]
> the patch
> 
> The patch for review. Thanks.

The major benefit of this patch is to save a physical register for certain operations, for example when we compare two non-speculated JS values, previously in the worst case we require 6 registers (two for each value, two for the result) - though we can reuse possible value registers for result registers - and it works before. But w/ the function inlining support in DFG there are more possibilities that the registers cannot be reused, which introduces problem on X86 where only 5 GPRs are available for now.

Performance is almost neutral.

[Filip Pizlo]

Comment on attachment 112166 [details]
the patch

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.

[WebKit Review Bot]

Comment on attachment 112166 [details]
the patch

Clearing flags on attachment: 112166

Committed r98291: <http://trac.webkit.org/changeset/98291>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yuqiang Xian]

(In reply to comment #4)
> (From update of attachment 112166 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112166&action=review
> 
> r=me.  The inline comments are suggestions only.
> 

Thanks for the suggestions. I think I can take a note here and have them addressed soon.