[Webkit-unassigned] [Bug 128115] [Win] LLINT is not working.

[bugzilla-daemon at webkit.org]

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


Mark Lam <mark.lam at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #223031|review?                     |review-
               Flag|                            |




--- Comment #9 from Mark Lam <mark.lam at apple.com>  2014-02-03 16:20:04 PST ---
(From update of attachment 223031)
View in context: https://bugs.webkit.org/attachment.cgi?id=223031&action=review

Good work.  I tried applying your 2nd patch and doing a Windows build, but the build failed in the generation of LowLevelInterpreterWin.asm.  Can you please address the feedback, rebase to ToT (if you haven't already), and retest to make sure that the patch does build for Windows?  Also make sure the EWS bots are green.  I'll take another look after that.  Thanks.

> Source/JavaScriptCore/ChangeLog:11
> +        The aim of this patch is to get LLINT it up and running on Windows.

typo: remove "it".

> Source/JavaScriptCore/offlineasm/asm.rb:188
> +        putsProcEndIfNeeded

Since this is only needed for isMSVC, please change this line to "putsProcEndIfNeeded if isMSVC".  The notion of ProcEnd has no meaning for non-MSVC code anyway, and making it explicit better mirrors the isMSVC condition call to putsProc() below.

> Source/JavaScriptCore/offlineasm/asm.rb:211
> +        putsProcEndIfNeeded

Is this really needed?  I would think that local labels should not exists out of the bounds of a PROC and ENDP.  If this indeed needed, can you provide details of an example where it's needed and why?

> Source/JavaScriptCore/offlineasm/x86.rb:801
> +                if !isIntelSyntax
> +                    $asm.puts "fucomi #{operands[1].x87Operand(0)}"
> +                else
> +                    $asm.puts "fucomi st(0), #{operands[1].x87Operand(0)}"
> +                end

This pattern seems to be repeated in many places.  Why don't you express this as:

    $asm.puts "fucomi #{@@floatingPointCompareImplicitOperand} #{operands[1].x87Operand(0)}"

where floatingPointCompareImplicitOperand is:
    @@floatingPointCompareImplicitOperand = isIntelSyntax ? "st(0), " : ""

… or something along those lines?

> Source/JavaScriptCore/offlineasm/x86.rb:808
> +                if !isIntelSyntax
> +                    $asm.puts "fucomip #{operands[1].x87Operand(1)}"
> +                else
> +                    $asm.puts "fucomip st(0), #{operands[1].x87Operand(1)}"
> +                end

ditto.

> Source/JavaScriptCore/offlineasm/x86.rb:816
> +                if !isIntelSyntax
> +                    $asm.puts "fucomi #{operands[0].x87Operand(0)}"
> +                else
> +                    $asm.puts "fucomi st(0), #{operands[0].x87Operand(0)}"
> +                end

ditto.

> Source/JavaScriptCore/offlineasm/x86.rb:823
> +                if !isIntelSyntax
> +                    $asm.puts "fucomip #{operands[0].x87Operand(1)}"
> +                else
> +                    $asm.puts "fucomip st(0), #{operands[0].x87Operand(1)}"
> +                end

ditto.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.