Bug 131612 – JavaScriptCore: ARM build fix after r167094.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[László Langó]

After r167094 there are many build errors on ARM like these:

    /tmp/ccgtHRno.s:370: Error: invalid constant (425a) after fixup
    /tmp/ccgtHRno.s:374: Error: invalid constant (426e) after fixup
    /tmp/ccgtHRno.s:378: Error: invalid constant (4282) after fixup
    /tmp/ccgtHRno.s:382: Error: invalid constant (4296) after fixup

Problem is caused by the wrong generated assembly like:
    "\tmov r2, (" LOCAL_LABEL_STRING(llint_op_strcat) " - " LOCAL_LABEL_STRING(relativePCBase) ")\n" // /home/webkit/WebKit/Source/JavaScriptCore/llint/LowLevelInterpreter.asm:741

`mov` can only move 8bit immediate, but not every constant fit into 8 bit. We should use `mow` rather than. CLang changes `mov` to `mow` automatically, but binutils doesn't.

[László Langó]

Created attachment 229278 [details]
Patch

[Peter Gal]

Comment on attachment 229278 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=229278&action=review

> Source/JavaScriptCore/ChangeLog:18
> +        `mov` can only move 8 bit immediate, but not every constant fit into 8 bit. We should use `movw` rather than.

The last sentence is a bit awkward for me.

> Source/JavaScriptCore/ChangeLog:19
> +        CLang changes `mov` to `movw` automatically, but binutils doesn't.

This doesn't mention the movt, which is also important. AFAIK clang converts the mov to a single movw or a movw and a movt, depending on the immediate.

> Source/JavaScriptCore/offlineasm/arm.rb:75
> +      @mov_counter ||= 0
> +      @mov_counter += 1
> +      const_label = "_mov_constant_label#{@mov_counter}"

Hmm... if we use @-s then it'll be an instance variable, and so we can end up with the same 'const_label' values. Maybe we can use $ so it'll be a global variable.
Or we could skip the whole counter, because it is valid to overwrite an assembly symbol with another .equ, but that approach could make the reading/understanding of the code bit harder.

[László Langó]

Created attachment 229283 [details]
Patch

[Darin Adler]

Comment on attachment 229283 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=229283&action=review

> Source/JavaScriptCore/offlineasm/arm.rb:78
> -      $asm.puts "mov #{register.armOperand}, (#{value})"
> +      $mov_counter ||= 0
> +      $mov_counter += 1
> +      const_label = "_mov_constant_label#{$mov_counter}"
> +      $asm.puts ".equ #{const_label}, (#{value})"
> +      $asm.puts "movw #{register.armOperand}, \#:lower16:#{const_label}"
> +      $asm.puts "movt #{register.armOperand}, \#:upper16:#{const_label}"

Won’t this hurt performance for constants that do fit into 8 bits, under clang at least?

[Csaba Osztrogonác]

Comment on attachment 229283 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=229283&action=review

>> Source/JavaScriptCore/offlineasm/arm.rb:78
>> +      $asm.puts "movw #{register.armOperand}, \#:lower16:#{const_label}"
>> +      $asm.puts "movt #{register.armOperand}, \#:upper16:#{const_label}"
> 
> Won’t this hurt performance for constants that do fit into 8 bits, under clang at least?

I checked the generated assembly after the proposed fix, and it seems all offsets 
fit in 16 bits, but none of them fit in 8 bits, so we can simple use movw always:

$asm.puts "movw #{register.armOperand}, #{const_label}"

Or we could ... if GNU assembler supported it ... but I got build error:
Error: constant expression expected -- `movw r2,_mov_constant_label118'

It's so strange ...
$asm.puts "movw #{register.armOperand}, \#:lower16:#{const_label}" works fine, but doesn't work without :lower16:

[Csaba Osztrogonác]

We can use this oneliner fix if we use :lower16:
-      $asm.puts "mov #{register.armOperand}, (#{value})"
+      $asm.puts "movw #{register.armOperand}, \#:lower16:(#{value})"

But it might cause strange failures in the future once 
the size of the constant becomes bigger than 16 bits.

[Csaba Osztrogonác]

Or we can use this to make sure we won't loose any information in the future:
-      $asm.puts "mov #{register.armOperand}, (#{value})"
+      $asm.puts "movw #{register.armOperand}, \#:lower16:(#{value})"
+      $asm.puts "movt #{register.armOperand}, \#:upper16:(#{value})"

I think this extra assembly instruction won't have significant
performance effect, because llint_entry runs only once to fill
the entry point table. Am I right? Michael? Mark?

[Michael Saboff]

(In reply to comment #7)
> Or we can use this to make sure we won't loose any information in the future:
> -      $asm.puts "mov #{register.armOperand}, (#{value})"
> +      $asm.puts "movw #{register.armOperand}, \#:lower16:(#{value})"
> +      $asm.puts "movt #{register.armOperand}, \#:upper16:(#{value})"
> 
> I think this extra assembly instruction won't have significant
> performance effect, because llint_entry runs only once to fill
> the entry point table. Am I right? Michael? Mark?

When I created the original patch, my research showed that mov is a pseudo op that the assembler would convert as needed to a single 8 or 16 bit immediate move or a movw/movt pair.  Certainly that is what we see with clang.  Gas/gcc should do the same.  It is also apparent that we need 16 bits (currently 15 bits) for llint_entry.

It is the case that llint_entry is intended to be run through this path only during startup.  However I don't want to change all arm immediate moves to be 16 bit though, as many, possible most of the llint immediate moves have values that fit in 8 bits.

Another way of handling this is to make the use of a 16 bit move predicated on an add or subtract of two labels and keep the current path for other cases.

[Michael Saboff]

Comment on attachment 229283 [details]
Patch

As explained in https://bugs.webkit.org/show_bug.cgi?id=131612#c8

[Csaba Osztrogonác]

(In reply to comment #8)
> (In reply to comment #7)
> When I created the original patch, my research showed that mov is a pseudo op that the assembler would convert as needed to a single 8 or 16 bit immediate move or a movw/movt pair.  Certainly that is what we see with clang.  Gas/gcc should do the same.  It is also apparent that we need 16 bits (currently 15 bits) for llint_entry.

gas can only convert mov rn, (label1 - label2) to T1 (imm8) 
and T2 (imm12) and can't convert to t3 (imm16). 

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0406c/index.html - A8.8.102 MOV (immediate)

Using movw and movt here won't affect other 8/16 bit immediate moves,
because string argument is only used for llint_entry now, nowhere else.
8/12/16 bits immediate values are handled after the string case:
http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/offlineasm/arm.rb#L70

> It is the case that llint_entry is intended to be run through this path only during startup.  However I don't want to change all arm immediate moves to be 16 bit though, as many, possible most of the llint immediate moves have values that fit in 8 bits.
> 
> Another way of handling this is to make the use of a 16 bit move predicated on an add or subtract of two labels and keep the current path for other cases.

I think we can add a new ARM specific offline assembler instruction
only for this llint_entry use case: move rn, (label1-label2) which
translated to movw and movt.

[László Langó]

Created attachment 229442 [details]
Patch

[Michael Saboff]

Comment on attachment 229442 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=229442&action=review

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:745
> +    elsif ARM or ARMv7 or ARMv7_TRADITIONAL
> +        mvlbl (label - _relativePCBase), t2
> +        addp t2, t1, t2
> +        move index, t3
> +        storep t2, [a0, t3, 4]
> +    elsif SH4

If the issue only manifests with ARM traditional (not Thumb2), could you make this "elsif ARM or ARMv7_TRADITIONAL"?

> Source/JavaScriptCore/offlineasm/arm.rb:489
> +        when "mvlbl"
> +                $asm.puts "movw #{operands[1].armOperand}, \#:lower16:#{operands[0].value}"
> +                $asm.puts "movt #{operands[1].armOperand}, \#:upper16:#{operands[0].value}"

I believe that the movw is sufficient.

[Csaba Osztrogonác]

(In reply to comment #12)
> (From update of attachment 229442 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229442&action=review
> 
> > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:745
> > +    elsif ARM or ARMv7 or ARMv7_TRADITIONAL
> > +        mvlbl (label - _relativePCBase), t2
> > +        addp t2, t1, t2
> > +        move index, t3
> > +        storep t2, [a0, t3, 4]
> > +    elsif SH4
> 
> If the issue only manifests with ARM traditional (not Thumb2), could you make this "elsif ARM or ARMv7_TRADITIONAL"?

No, it isn't ARM traditional issue. The bug is valid on 
ARM traditional and ARM Thumb2 instruction sets too. 

> > Source/JavaScriptCore/offlineasm/arm.rb:489
> > +        when "mvlbl"
> > +                $asm.puts "movw #{operands[1].armOperand}, \#:lower16:#{operands[0].value}"
> > +                $asm.puts "movt #{operands[1].armOperand}, \#:upper16:#{operands[0].value}"
> 
> I believe that the movw is sufficient.

It is sufficient now. But what if once the difference of offsets 
become bigger than 16 bit? It would cause obviously wrong behaviour.
Unfortunately there isn't a good way to determine if this immediate
fits to 16 bit or not in build time.

[Csaba Osztrogonác]

Comment on attachment 229442 [details]
Patch

Clearing flags on attachment: 229442

Committed r167566: <http://trac.webkit.org/changeset/167566>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.

[Filip Pizlo]

Comment on attachment 229442 [details]
Patch

You shouldn't be adding new instructions for this.  It's straightforward to modify the lowering of "move" to realize that on ARM we need to emit the movw/movt pair in case of label differences.  Second, if you're going to add an instruction for moving labels (er, label differences) into registers, then you should change setEntryAddress to use this instruction on all architectures and not make it ARM-specific.  This would allow you to remove some redundant code.

In the future, please put some thought into how to make your changes minimal.  This is important for avoiding bugs in the future.

[cand]

This patch broke the ARMv6 build.

movw and movt are not available on v6, and other places use ldr instead, with a check for v7.

https://github.com/clbr/webkitfltk/issues/7

[Csaba Osztrogonác]

(In reply to comment #17)
> This patch broke the ARMv6 build.
> 
> movw and movt are not available on v6, and other places use ldr instead,
> with a check for v7.
> 
> https://github.com/clbr/webkitfltk/issues/7

new bug report for it: bug141288