[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

--- Comment #18 from Caio Lima <ticaiolima at gmail.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

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3525
>> +    // 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.

Nice point. I totally agree with that. The current implementation already reuses the result.tagGPR as scratchGPR. I am going to move it to Generators and remove this allocation here.

>> 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.

Actually, the correct semantic behavior is 0. If you check current implementation "patchpoint->numFPScratchRegisters = 2" because leftFPR and rightFPR are allocated using "patchpoint->numFPScratchRegisters". That was the reason that in my last Patch I was using "patchpoint->numFPScratchRegisters = 2 + numFPScratchRegisters;". I think that is better the way that I implemented before, because it is going to keep numFPScratchRegisters consistent between DFG and FTL layers.

>> Source/JavaScriptCore/jit/JITMathIC.h:55
>> +#define MATH_IC_MAX_FPR 3
> 
> Which IC uses 3?

It is because the MathIC for B3 allocates right, left and scratch FPRs using "patchpoint->numFPScratchRegisters = numFPScratchRegisters". For all generators scratchFPR are always InvalidFPRReg.

-- 
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/20160925/6127f340/attachment-0001.html>