Bug 183286 – [ARM] Assembler warnings: "use of r13 is deprecated"

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Dominik Inführ]

While building JSC on ARM the assembler prints warning messages: "/tmp/ccJLuS3e.s:58: use of r13 is deprecated". Use of operand r13 in Rm (the second operand) is deprecated in ARM. Prevent this in the offlineasm/arm.rb by switching the operands if sp occurs as the second operand.

[Dominik Inführ]

Created attachment 334895 [details]
Patch

[Mark Lam]

Comment on attachment 334895 [details]
Patch

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

> Source/JavaScriptCore/offlineasm/arm.rb:230
> +                when "aeq" then "beq"

Shouldn't the opposite of >= be < i.e "b", not "beq"?  Please double check.

> Source/JavaScriptCore/offlineasm/arm.rb:232
> +                when "beq" then "aeq"

Ditto: opposite of "beq" is "a"

> Source/JavaScriptCore/offlineasm/arm.rb:234
> +                when "gteq" then "lteq"

Ditto: opposite of "gteq" is "lt".

> Source/JavaScriptCore/offlineasm/arm.rb:236
> +                when "lteq" then "gteq"

Ditto: opposite of "lteq" is "gt".

[Dominik Inführ]

Hi, thanks for the review! If I am not mistaken, we don't need the opposite of a comparison here. What I am trying to do is replacing "branch if a >= sp to L1" with "branch if sp <= a to L1". I assume the problem is that I chose a pretty terrible name "armNegateOpcode" that implies negating the operation. I will change the name of this method to something more sensible.

[Dominik Inführ]

Created attachment 335097 [details]
Patch

[Dominik Inführ]

I've now renamed the method to "armOpcodeReversedOperands".

[Mark Lam]

Comment on attachment 335097 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 335097 [details]
Patch

Rejecting attachment 335097 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 335097, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
st, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
Auto packing the repository in background for optimum performance.
See "git help gc" for manual housekeeping.
error: The last gc run reported the following. Please correct the root cause
and remove /Volumes/Data/EWS/WebKit/.git/gc.log.
Automatic cleanup will not be performed until the file is removed.

warning: There are too many unreachable loose objects; run 'git prune' to remove them.


Full output: http://webkit-queues.webkit.org/results/6829021

[Mark Lam]

Comment on attachment 335097 [details]
Patch

let's try the commit queue again.

[Filip Pizlo]

Comment on attachment 335097 [details]
Patch

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

> Source/JavaScriptCore/offlineasm/arm.rb:239
> +    operation = case m.post_match
> +                when "eq" then "eq"
> +                when "neq" then "neq"
> +                when "a" then "b"
> +                when "aeq" then "beq"
> +                when "b" then "a"
> +                when "beq" then "aeq"
> +                when "gt" then "lt"
> +                when "gteq" then "lteq"
> +                when "lt" then "gt"
> +                when "lteq" then "gteq"
> +                else
> +                    raise "unknown operation #{m.post_match}"
> +                end

This is super weird style.

Please stick to 4 space indent.

[Mark Lam]

(In reply to Mark Lam from comment #8)
> Comment on attachment 335097 [details]
> Patch
> 
> let's try the commit queue again.

Actually, it looks like the commit was successful.  The patch was landed in r229331: <http://trac.webkit.org/r229331>.

[Mark Lam]

(In reply to Filip Pizlo from comment #9)
> Comment on attachment 335097 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335097&action=review
> 
> > Source/JavaScriptCore/offlineasm/arm.rb:239
> > +    operation = case m.post_match
> > +                when "eq" then "eq"
> > +                when "neq" then "neq"
> > +                when "a" then "b"
> > +                when "aeq" then "beq"
> > +                when "b" then "a"
> > +                when "beq" then "aeq"
> > +                when "gt" then "lt"
> > +                when "gteq" then "lteq"
> > +                when "lt" then "gt"
> > +                when "lteq" then "gteq"
> > +                else
> > +                    raise "unknown operation #{m.post_match}"
> > +                end
> 
> This is super weird style.
> 
> Please stick to 4 space indent.

Dominik, please do a follow up patch to fix the style issues that Fil identified.

[Dominik Inführ]

Hi, thanks for reviewing and getting it through the commit queue. I've created a bug/patch to fix the indentation here: https://bugs.webkit.org/show_bug.cgi?id=183400.

[Mark Lam]

The indentation has been fixed in r229356: <https://trac.webkit.org/r229356> in https://bugs.webkit.org/show_bug.cgi?id=183400.

[Radar WebKit Bug Importer]

<rdar://problem/38220239>