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

[bugzilla-daemon at webkit.org]

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





--- Comment #18 from Mark Lam <mark.lam at apple.com>  2014-02-05 11:53:59 PST ---
(From update of attachment 223229)
View in context: https://bugs.webkit.org/attachment.cgi?id=223229&action=review

Thanks for the update.  Still need some fixes.  My suggestions (nits) and the issues that need fixing are detailed below.

I also like Geoff’s recommendation.  You should be able to enhance asm.rb to generate the symbol asm file if isMSVC.  Either make the symbol filename an extra optional command line arg (at the end) to asm.rb, or synthesize it from the LowLevelInterpreterWin.asm name.  My concern here is to not break other ports.

Also, when you’re done with your new patch, please run the jsc tests to make sure that your patch is working properly:
$ pathToYourOpenSourceDir/Tools/Scripts/run-javascriptcore-test --debug --no-build --no-jsc-stress

For extra credit, also run the layout tests and make sure your patch is not breaking anything new:
$ pathToYourOpenSourceDir/Tools/Scripts/run-webkit-test --debug --no-build

Thanks.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1503
>  macro noAdditionalChecks(oldStructure, scratch)

change to: macro noAdditionalChecks(oldStructure, scratch, slowPath)

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1524
> +
> +    .opPutByIdSlow:
> +        callSlowPath(_llint_slow_path_put_by_id)
> +        dispatch(9)

Remove this blob (it is a replica of the one provided by putByIdTransition().  Instead:
1. Change structureChainChecks() to: macro structureChainChecks(oldStructure, scratch, slowPath)
2. Change putByIdTransition() to call additionalChecks(t1, t3, .opPutByIdSlow)
3. Change structureChainChecks() to use the passed in slowPath arg instead of .opPutByIdSlow.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:-212
> -        storei t0, 0xbbadbeef[]

Why remove this line?

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:-699
> -.functionForCallBegin:
> -    functionInitialization(0)
> -    dispatch(0)

This is not functionally equivalent, and hence is a bug.  _llint_function_for_call_prologue expects to fall thru into .functionForCallBegin.  Now it doesn’t.

Fix it by removing the .functionCallBegin label (which isn’t used locally).  I guess will have to replicate this code.  I’ll do some benchmarking after this to make sure that there’s no perf impact here.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:-706
>  _llint_function_for_construct_prologue:
>      prologue(functionForConstructCodeBlockGetter, functionCodeBlockSetter, _llint_entry_osr_function_for_construct, _llint_trace_prologue_function_for_construct)
> -.functionForConstructBegin:
> -    functionInitialization(1)
> -    dispatch(0)

This is not functionally equivalent, and hence is a bug.  _llint_function_for_construct_prologue expects to fall thru into .functionForConstructBegin.  Now it doesn’t.

Fix by removing the .functionForConstructBegin label which isn’t used locally.

> Source/JavaScriptCore/offlineasm/asm.rb:43
> +def commentString
> +    if !isMSVC
> +        "//"
> +    else
> +        ";"
> +    end
> +end

nit: rename commentString to commentPrefix.

def commentPrefix
    isMSVC ? “;” : “//"
end

> Source/JavaScriptCore/offlineasm/asm.rb:64
> -        @outp.puts "OFFLINE_ASM_BEGIN"
> +        if !isMSVC
> +            @outp.puts "OFFLINE_ASM_BEGIN"
> +        end

nit: @outp.puts “OFFLINE_ASM_BEGIN” if !isMSVC

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

putsProcEndIfNeeded if isMSVC

> Source/JavaScriptCore/offlineasm/asm.rb:73
> +        if !isMSVC
> +            @outp.puts "OFFLINE_ASM_END"
> +        end

nit: @outp.puts “OFFLINE_ASM_END” if !isMSVC

> Source/JavaScriptCore/offlineasm/asm.rb:101
> -            result = "// " + result
> +            result = commentString + " " + result

nit: rename commentString to commentPrefix.

> Source/JavaScriptCore/offlineasm/asm.rb:173
> +    def putsProc(label, comment)

Add a line after the def here to ensure that this is only called for MSVC:

    raise unless isMSVC

> Source/JavaScriptCore/offlineasm/asm.rb:178
> +    def putsProcEndIfNeeded

Add a line after the def here to ensure that this is only called for MSVC:

    raise unless isMSVC

> Source/JavaScriptCore/offlineasm/asm.rb:256
> +                @outp.puts "    " + commentString + " #{@codeOrigin}"
> +                @outp.puts "    " + commentString + " #{text}"

nit:
@outp.puts "    " + commentPrefix + “ #{@codeOrigin}”
@outp.puts "    " + commentPrefix + “ #{text}"

> Source/JavaScriptCore/offlineasm/asm.rb:289
> +    commentString + " offlineasm input hash: " + parseHash(asmFile) +

nit: commentPrefix + “ offlineasm input hash: " + parseHash(asmFile) +

> Source/JavaScriptCore/offlineasm/x86.rb:70
> +    if isIntelSyntax
> +        name        
> +    else
> +        "%" + name
> +    end

Change to: isIntelSyntax ? name : “%” + name

> Source/JavaScriptCore/offlineasm/x86.rb:78
> +    if isIntelSyntax
> +        "[#{off} + #{register}]"
> +    else
> +        "#{off}(#{register})"
> +    end

Change to: isIntelSyntax ? "[#{off} + #{register}]” : "#{off}(#{register})"

> Source/JavaScriptCore/offlineasm/x86.rb:86
> +    if isIntelSyntax
> +        ""
> +    else
> +        "*"
> +    end

Change to: isIntelSyntax ? “” : “*"

> Source/JavaScriptCore/offlineasm/x86.rb:94
> +    if isIntelSyntax
> +        "#{opB}, #{opA}"
> +    else
> +        "#{opA}, #{opB}"
> +    end

Change to: isIntelSyntax ? "#{opB}, #{opA}” : "#{opA}, #{opB}"

> Source/JavaScriptCore/offlineasm/x86.rb:102
> +    if isIntelSyntax
> +        "#{c}"
> +    else
> +        "$#{c}"
> +    end

Change to: isIntelSyntax ? "#{c}” : "$#{c}"

> Source/JavaScriptCore/offlineasm/x86.rb:794
> +        @@floatingPointCompareImplicitOperand = isIntelSyntax ? "st(0), " : ""

I think you can put this at the top of the Instruction class so that you can use it in the other remaining emission of fucomip below.

> Source/JavaScriptCore/offlineasm/x86.rb:1091
> -                $asm.puts "fildl -4(#{sp.x86Operand(:ptr)})"
> -                $asm.puts "fucomip #{operands[0].x87Operand(1)}"
> +                $asm.puts "fild#{x86Suffix(:int)} #{getSizeString(:int)}#{offsetRegister(-4, sp.x86Operand(:ptr))}"
> +                if !isIntelSyntax
> +                    $asm.puts "fucomip #{operands[0].x87Operand(1)}"
> +                else
> +                    $asm.puts "fucomip st(0), #{operands[0].x87Operand(1)}"
> +                end

Use @@floatingPointCompareImplicitOperand here.

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