[Webkit-unassigned] [Bug 160588] Refactor MathIC compilation process in Baseline and DFG to turn temporary registers usage more flexible

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=160588

Saam Barati <sbarati at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #289565|review?                     |review-
              Flags|                            |

--- Comment #17 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 289565
  --> https://bugs.webkit.org/attachment.cgi?id=289565
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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160924/15763dab/attachment-0001.html>