Bug 183286 - [ARM] Assembler warnings: "use of r13 is deprecated"
Summary: [ARM] Assembler warnings: "use of r13 is deprecated"
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 183400
Blocks:
  Show dependency treegraph
 
Reported: 2018-03-02 05:03 PST by Dominik Inführ
Modified: 2018-03-07 07:16 PST (History)
11 users (show)

See Also:


Attachments
Patch (3.24 KB, patch)
2018-03-02 05:25 PST, Dominik Inführ
no flags Details | Formatted Diff | Diff
Patch (3.24 KB, patch)
2018-03-06 08:24 PST, Dominik Inführ
fpizlo: review-
fpizlo: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominik Inführ 2018-03-02 05:03:22 PST
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.
Comment 1 Dominik Inführ 2018-03-02 05:25:02 PST
Created attachment 334895 [details]
Patch
Comment 2 Mark Lam 2018-03-06 07:35:55 PST
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".
Comment 3 Dominik Inführ 2018-03-06 08:08:06 PST
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.
Comment 4 Dominik Inführ 2018-03-06 08:24:15 PST
Created attachment 335097 [details]
Patch
Comment 5 Dominik Inführ 2018-03-06 08:27:02 PST
I've now renamed the method to "armOpcodeReversedOperands".
Comment 6 Mark Lam 2018-03-06 09:25:36 PST
Comment on attachment 335097 [details]
Patch

r=me
Comment 7 WebKit Commit Bot 2018-03-06 11:14:15 PST
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
Comment 8 Mark Lam 2018-03-06 11:20:23 PST
Comment on attachment 335097 [details]
Patch

let's try the commit queue again.
Comment 9 Filip Pizlo 2018-03-06 11:32:28 PST
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.
Comment 10 Mark Lam 2018-03-06 14:30:46 PST
(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>.
Comment 11 Mark Lam 2018-03-06 14:31:46 PST
(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.
Comment 12 Dominik Inführ 2018-03-07 01:09:43 PST
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.
Comment 13 Mark Lam 2018-03-07 07:15:20 PST
The indentation has been fixed in r229356: <https://trac.webkit.org/r229356> in https://bugs.webkit.org/show_bug.cgi?id=183400.
Comment 14 Radar WebKit Bug Importer 2018-03-07 07:16:27 PST
<rdar://problem/38220239>