Aarch64 - improve after lea lowering #7929

swalk-cavium · 2017-07-25T17:16:25Z

This change improves the sequence generated when lowering the Vptr element of the lea
Vinstr by taking advantage of the sub immediate instruction. This saves 1 instruction
per instance.

Before

(07) StLocRange<[0, 108)> t0:FramePtr, Uninit
    Main:   
        0x52c0035c  d10043a0              sub x0, x29, #0x10 (16)
        0x52c00360  9280d9e1              movn x1, #0x6cf
        0x52c00364  8b0103a1              add x1, x29, x1 
        0x52c00368  3900201f              strb wzr, [x0, #8]

After

(07) StLocRange<[0, 108)> t0:FramePtr, Uninit
    Main:   
        0x50400404  d10043a0              sub x0, x29, #0x10 (16)
        0x50400408  d11b43a1              sub x1, x29, #0x6d0 (1744)  //<<---
        0x5040040c  3900201f              strb wzr, [x0, #8]

This was seen approximately 300 times in hphp/test/quick/all_type_comparison_test.php
and around 1000 times in hphp/test/zend/good/ext/intl/tests/grapheme.php

The standard regression tests were run with six option sets. No new failures were observed.

swalk-cavium · 2017-07-25T17:19:14Z

@mxw - Hi Max, Can you inspect this one?

jim-saxman · 2017-07-25T22:21:22Z

@swalk-cavium No new failures in either the unit tests or in OSS Performance suite.

mxw · 2017-08-17T03:12:57Z

hphp/runtime/vm/jit/vasm-simplify-arm.cpp

@@ -99,6 +99,25 @@ bool simplify(Env& env, const loadb& inst, Vlabel b, size_t i) {

 ///////////////////////////////////////////////////////////////////////////////

+bool simplify(Env& env, const ldimmq& inst, Vlabel b, size_t i) {
+  return if_inst<Vinstr::lea>(env, b, i + 1, [&] (const lea& ea) {
+    // ldimmq{s, tmp}; lea{tmp, d} -> subqi{s, d}


This description makes no sense. ldimmq{} has a Vreg dst, while lea{} has a Vptr src; those are incompatible.

Please update this as follows:

// ldimmq{s, index}; lea{base[index], d} -> lea{base[s], d}

mxw · 2017-08-17T03:16:51Z

hphp/runtime/vm/jit/vasm-simplify-arm.cpp

+      auto sf = env.unit.makeReg();
+      v << subqi{-inst.s.l(), ea.s.base, ea.d, sf};
+      return 2;
+    });


What is this simplification intended to do? Currently, it could transform the following sequence:

ldimmq{-42, rimm}; lea{rvmfp()[0], d}; store{rimm, m};

into:

subqi{42, rvmfp(), d, sf}; store{rimm, m};

This is totally nonsensical and unsound, and I'm not really sure what you meant to do here.

swalk-cavium · 2017-08-17T20:11:33Z

@mxw - Hi Max, the code is intended to detect this sequence

Initial generation

lea { Vptr{baseReg, DISP}, Vreg64{leaDstReg} }

After lowerVptr

ldimmq {DISP, newReg}
lea { Vptr{baseReg, newReg}, Vreg64{leaDstReg} }

After emission

movn newReg, -DISP
add leaDstReg, baseReg, newReg

When the final sequence could really be one
subtract instruction.

I think you're right. The conditions could be tighter to not fire in your
example. I'll update the request after testing finishes.

facebook-github-bot · 2017-08-18T20:53:50Z

@swalk-cavium updated the pull request - view changes

swalk-cavium · 2017-08-18T20:58:25Z

@mxw - Hi Max, I think this corrects the issue you noted. Regression test run with six
options sets. No additional failures. Thanks for finding it.

mxw · 2017-08-18T21:11:20Z

@swalk-cavium—Thinking on this a little more:

Can this go in the imm-folder instead of in simplify?
If not, why not? And can we rewrite the lea{} as an lea{} with immediate displacement instead of a subqi{}, to avoid setting the flags register?

swalk-cavium · 2017-08-18T21:22:35Z

@mxw - Hi Max, I don't think this could be caught in the imm-folder. The immediate values used there are coming from the Vunit

554 // figure out which Vregs are constants and stash their values.
555 for (auto& entry : unit.constToReg) {
556 folder.valid.set(entry.second);
557 folder.vals[entry.second] = entry.first.val;
558 }
runtime/vm/jit/vasm-fold-imms.cpp

Won't the VisitSF pass take care of the status flags?

mxw · 2017-08-19T02:10:26Z

Maybe the ImmFolder should also look for ldimm instructions.

The flags strength-reduction pass would take care of this, but in general, optimization passes should avoid dependencies on one another. Why use a subqi{} when an lea{} is strictly weaker and does everything you need?

swalk-cavium · 2017-08-21T18:58:33Z

@mxw - Hi Max, This peephole is needed because lea cannot do everything we need.

The extra instruction (movn in this case) is introduced during the lowering of the Vptr
in the initial lea instruction. lowerForARM --> lower(..., lea&, ...) --> lowerVptr().
I don't think we should pull the addressing logic into lower(..., lea&,...) function.

I think updating lowerVptr() to put the ldimmq immediates into the Vunit would have a
negative impact because it could potentially increase the live range. Right now the
range is only 1 instruction.

Updating the lea emitter is too late since the instruction creating the immediate has
already generated.

The lea instruction is emitted as an 'add rD,rN,uimm12'. There is no form for a signed
value. There is a 'sub rD,rN,uimm12' instruction which is what this is trying to utilize.

hhvm already generates the subqi in other contexts including the folder.

mxw · 2017-08-23T22:07:20Z

Can you change the lea{} emitter to emit either an add or a sub depending on whether the displacement is negative?

swalk-cavium · 2017-08-23T22:37:36Z

@mxw - Hi Max. By the time you get to the emitter the constant is not available. lowerVptr() has
changed it into a register with a mov immediate.

mxw · 2017-08-23T23:53:35Z

I realize that what you're doing here is to rewrite an lea{} with a 0 displacement and a nontrivial constant-valued index register as a constant displacement. My question is—why can't you implement that constant displacement via an lea{}, instead of via a sub{} of some sort?

swalk-cavium · 2017-08-24T15:36:57Z

@mxw - Hi Max, by the time you get to the emitter the constant is no longer available.
The lowerVptr() has removed constants outside the range of a simm9 into a separate register.
That range is the common denominator for all the addressing modes.

Are you suggesting making a special case of lower(..., lea&, ...) ? In that case it would still have
to know some specifics of the Vptr internals like the peephole.

facebook-github-bot · 2017-08-25T14:20:56Z

@mxw has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-08-28T15:31:07Z

@swalk-cavium updated the pull request - view changes - changes since last import

swalk-cavium · 2017-08-28T15:33:18Z

@mxw - Hi Max, Updated per our extended IRC last week. Ran regression tests 6 option sets, no new regressions. Examined hphp.log and it cleans up the extra movn instructions as expected.

mxw · 2017-08-28T17:20:15Z

hphp/runtime/vm/jit/vasm-simplify-arm.cpp

+    // ldimmq{s, tmp}; lea{tmp, d} -> subqi{s, d}
+    if (!(env.use_counts[inst.d] == 1 &&
+          inst.s.q() <= -1  &&
+          inst.s.q() >= -4095 &&


Are positive values up through 4095 not also suitable for this optimization?

mxw · 2017-08-28T17:20:55Z

hphp/runtime/vm/jit/vasm-simplify-arm.cpp

+    return simplify_impl(env, b, i, [&] (Vout& v) {
+      // eXtend ea - lowerVptr() too conservative.
+      Vptr xea{ea.s.base, inst.s.l()};
+      v << lea{xea, ea.d};


Just

v << lea{ea.s.base[inst.s.l()], ea.d};

This notation reads more clearly than the opaque Vptr constructor.

mxw · 2017-08-28T17:21:54Z

hphp/runtime/vm/jit/vasm-simplify-arm.cpp

@@ -99,6 +99,25 @@ bool simplify(Env& env, const loadb& inst, Vlabel b, size_t i) {

 ///////////////////////////////////////////////////////////////////////////////

+bool simplify(Env& env, const ldimmq& inst, Vlabel b, size_t i) {
+  return if_inst<Vinstr::lea>(env, b, i + 1, [&] (const lea& ea) {
+    // ldimmq{s, tmp}; lea{tmp, d} -> subqi{s, d}


Please update this as follows:

// ldimmq{s, index}; lea{base[index], d} -> lea{base[s], d}

facebook-github-bot · 2017-08-29T17:32:23Z

@swalk-cavium updated the pull request - view changes - changes since last import

swalk-cavium · 2017-08-29T17:33:20Z

@mxw - Updated per comments, retested.

swalk-cavium · 2017-09-07T14:54:13Z

@mxw - Hi Max, Any further comments on this one?

facebook-github-bot · 2017-09-08T15:18:49Z

@mxw has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

swalk-cavium · 2017-09-27T18:41:20Z

@mxw - Hi Max, Any further comments on this one?

hhvm-bot

@mxw is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

swalk-cavium · 2017-10-16T16:41:59Z

@max - Hi Max, I retested this one this morning. It still saves 1 instruction whenever it fires.
Anything else I need to do?

hhvm-bot

@mxw is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Aarch64 - improve after lea lowering

47e1509

hhvm-bot added CLA Signed GH Review: review-needed labels Jul 25, 2017

mofarrell assigned mxw Jul 28, 2017

mxw suggested changes Aug 17, 2017

View reviewed changes

Aarch64 - improve after lea lowering (post inspection)

10330a9

mxw added GH Review: needs-revision and removed GH Review: review-needed labels Aug 18, 2017

hhvm-bot added the Import Started label Aug 25, 2017

Aarch64 - improve after lea lowering (lowerVptr too aggressive)

a7078b8

hhvm-bot added GH Review: review-needed and removed GH Review: needs-revision labels Aug 28, 2017

mxw suggested changes Aug 28, 2017

View reviewed changes

mxw added the GH Review: needs-revision label Aug 28, 2017

mxw removed the GH Review: review-needed label Aug 28, 2017

Aarch64 - improve after lea lowering

d5e060c

hhvm-bot added GH Review: review-needed and removed GH Review: needs-revision labels Aug 29, 2017

hhvm-bot approved these changes Oct 3, 2017

View reviewed changes

hhvm-bot approved these changes Feb 12, 2018

View reviewed changes

hhvm-bot closed this in 57cf791 Feb 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aarch64 - improve after lea lowering #7929

Aarch64 - improve after lea lowering #7929

swalk-cavium commented Jul 25, 2017

swalk-cavium commented Jul 25, 2017

jim-saxman commented Jul 25, 2017 •

edited

Loading

mxw Aug 17, 2017

mxw Aug 28, 2017

mxw Aug 17, 2017

swalk-cavium commented Aug 17, 2017

facebook-github-bot commented Aug 18, 2017

swalk-cavium commented Aug 18, 2017

mxw commented Aug 18, 2017

swalk-cavium commented Aug 18, 2017

mxw commented Aug 19, 2017

swalk-cavium commented Aug 21, 2017

mxw commented Aug 23, 2017

swalk-cavium commented Aug 23, 2017

mxw commented Aug 23, 2017

swalk-cavium commented Aug 24, 2017

facebook-github-bot commented Aug 25, 2017

facebook-github-bot commented Aug 28, 2017

swalk-cavium commented Aug 28, 2017

mxw Aug 28, 2017

mxw Aug 28, 2017

mxw Aug 28, 2017

facebook-github-bot commented Aug 29, 2017

swalk-cavium commented Aug 29, 2017

swalk-cavium commented Sep 7, 2017

facebook-github-bot commented Sep 8, 2017

swalk-cavium commented Sep 27, 2017

hhvm-bot left a comment

swalk-cavium commented Oct 16, 2017

hhvm-bot left a comment

Aarch64 - improve after lea lowering #7929

Aarch64 - improve after lea lowering #7929

Conversation

swalk-cavium commented Jul 25, 2017

Before

After

swalk-cavium commented Jul 25, 2017

jim-saxman commented Jul 25, 2017 • edited Loading

mxw Aug 17, 2017

Choose a reason for hiding this comment

mxw Aug 28, 2017

Choose a reason for hiding this comment

mxw Aug 17, 2017

Choose a reason for hiding this comment

swalk-cavium commented Aug 17, 2017

Initial generation

After lowerVptr

After emission

facebook-github-bot commented Aug 18, 2017

swalk-cavium commented Aug 18, 2017

mxw commented Aug 18, 2017

swalk-cavium commented Aug 18, 2017

mxw commented Aug 19, 2017

swalk-cavium commented Aug 21, 2017

mxw commented Aug 23, 2017

swalk-cavium commented Aug 23, 2017

mxw commented Aug 23, 2017

swalk-cavium commented Aug 24, 2017

facebook-github-bot commented Aug 25, 2017

facebook-github-bot commented Aug 28, 2017

swalk-cavium commented Aug 28, 2017

mxw Aug 28, 2017

Choose a reason for hiding this comment

mxw Aug 28, 2017

Choose a reason for hiding this comment

mxw Aug 28, 2017

Choose a reason for hiding this comment

facebook-github-bot commented Aug 29, 2017

swalk-cavium commented Aug 29, 2017

swalk-cavium commented Sep 7, 2017

facebook-github-bot commented Sep 8, 2017

swalk-cavium commented Sep 27, 2017

hhvm-bot left a comment

Choose a reason for hiding this comment

swalk-cavium commented Oct 16, 2017

hhvm-bot left a comment

Choose a reason for hiding this comment

jim-saxman commented Jul 25, 2017 •

edited

Loading