Aarch64 - update EqDbl to use SIMD/FP instructions #7914

swalk-cavium · 2017-07-12T14:44:18Z

This change updates the EqDbl and NeqDbl sequences to use different instructions.
It reduces the number of instructions needed and removes the dependency on the
status register.

Before

(29) t8:Bool = EqDbl t7:Dbl, t5:Dbl
    Main:   
          0x51003aec  d53b4210              mrs x16, nzcv
          0x51003af0  1e602020              fcmp d1, d0
          0x51003af4  da9f13f2              csetm x18, eq 
          0x51003af8  9e670240              fmov d0, x18 
          0x51003afc  d51b4210              msr nzcv, x16
          0x51003b00  9e660000              fmov x0, d0
          0x51003b04  53001c00              uxtb w0, w0
          0x51003b08  12000000              and w0, w0, #0x1

After

(29) t8:Bool = EqDbl t7:Dbl, t5:Dbl
    Main:   
          0x1460407c  5e60e420              fcmeq d0, d1, d0
          0x14604080  9e660000              fmov x0, d0
          0x14604084  12000000              and w0, w0, #0x1

This also updates the vixl disassembler to recognize two new instruction formats:
Advanced SIMD Scalar Three Same, and Advanced SIMD Scalar two-register Misc.
Whenever the enumeration clashed with an existing definition a prefix was added.
See STS, and STM respectively.

The regression suite was run with 6 option sets. No additional failures were observed.

swalk-cavium · 2017-07-13T21:34:08Z

@mxw - Hi Max, Can you take a look at this one? Thanks.

mxw

I just have the one comment. I'll leave it to one of @cmuellner, @dave-estes, @jim-saxman, and @apinski-cavium to vet the meat of the PR.

mxw · 2017-07-24T20:22:43Z

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

@@ -1545,7 +1533,7 @@ void lower(const VLS& e, movtdb& i, Vlabel b, size_t z) {
  lower_impl(e.unit, b, z, [&] (Vout& v) {
    auto d = v.makeReg();
    v << copy{i.s, d};
-    v << movtqb{d, i.d};
+    v << copy{d, i.d};


What's the purpose of copying through a tmp reg?

@mxw - The code generator wouldn't accept it without the extra copy. Something to do with changing register classes I suspect.

But the register widths are propagated through copies, and they already don't match since this is a truncation operation...

Can you paste the exact error?

@mxw - Looks like I didn't capture that in my daily log. I'll have to try and recreate.

@mxw - I was unable to recreate the issue. I'll reduce the copy and retest.

hhvm-bot · 2017-07-24T20:27:05Z

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

jim-saxman · 2017-07-25T15:48:20Z

Hi @swalk-cavium, This patch doesn't create any more unit test failures, and it passed all of OSS performance test suite.

hhvm-bot · 2017-07-26T15:04:03Z

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

swalk-cavium · 2017-07-26T15:06:19Z

@mxw - MOP results the same with only 1 copy, so updated pull request.

facebook-github-bot · 2017-08-17T02:48:17Z

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

mxw · 2017-08-17T03:24:56Z

Before I accept this, I'd like someone else to give a second opinion on the change (not just on test results) to confirm it doesn't affect the behavior of the instruction in any way.

swalk-cavium · 2017-08-17T16:23:36Z

@mxw - Hi Max, Here's how I tested the sequences before incorporating in hhvm
https://pastebin.com/PC6Ym7ZY

swalk-cavium · 2017-08-17T16:30:21Z

@mxw - Here's the table I used for the unordered comparison case.
https://pastebin.com/psgHNebB

SNaN - signalling NaN,
QNaN - quiet NaN

dave-estes-QCOM

This is a slick optimization. It's not super-intuitive, but a few comments will sort that out quickly.

dave-estes-QCOM · 2017-08-18T19:30:15Z

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

-    auto d = v.makeReg();
-    v << copy{i.s, d};
-    v << movtqb{d, i.d};
+    v << copy{i.s, i.d};


Is it safe to do this if we end up reverting f9ecdf1? Might want to split this change out.

@dave-estes - Hi Dave, I'm not sure. If your new version goes in first, I'll have to retest.

dave-estes-QCOM · 2017-08-18T19:35:23Z

hphp/vixl/a64/assembler-a64.cc

@@ -1352,6 +1352,22 @@ void Assembler::frintz(const FPRegister& fd,
 }


+void Assembler::fcmeq(const FPRegister& fd,


I think it would be more readable if this was named Assembler::fcmeqz().

dave-estes-QCOM · 2017-08-18T19:37:35Z

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

@@ -909,29 +909,17 @@ void Vgen::emit(const unpcklpd& i) {
 ///////////////////////////////////////////////////////////////////////////////

 void Vgen::emit(const cmpsd& i) {


I think renaming the 2 argument fcmeq (see next comment) will help, but I also think a line or two of comments will make this more clear too.

swalk-cavium · 2017-08-18T21:28:00Z

@mxw - I think I might have to update this to make it check the hardware capabilities. I just noticed at least one of the forms says, ARMv8.2. Something similar to what we did for the LSE Atomics.

swalk-cavium · 2017-08-18T22:09:17Z

@mxw - Um, disregard the previous comment. SIMD instructions are required in the
ARM SBSA (Server Base System Architecture).

facebook-github-bot · 2017-08-23T16:35:33Z

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

facebook-github-bot · 2017-08-23T22:01:56Z

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

mxw · 2017-08-23T22:02:13Z

@swalk-cavium—Cool, thanks for this change. I'll land it as soon as internal tests pass.

facebook-github-bot · 2017-08-24T15:39:08Z

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

swalk-cavium · 2017-12-04T22:39:53Z

@mxw - Hi Max, Any word on your testing infrastructure issue? Can this one land?

Aarch64 - update EqDbl to use SIMD/FP instructions

ce73afd

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

mxw self-assigned this Jul 24, 2017

mxw reviewed Jul 24, 2017

View reviewed changes

hhvm-bot added the Import Started label Jul 24, 2017

Aarch64 - update EqDbl to use SIMD/FP instructions (post inspection

abdab0b

mxw mentioned this pull request Aug 17, 2017

Aarch64 - improve emitter for shl #7921

Closed

dave-estes-QCOM reviewed Aug 18, 2017

View reviewed changes

Aarch64 - update EqDbl to use SIMD/FP instructions (another inspection)

afb4f5e

hhvm-bot added GH Review: accepted and removed GH Review: review-needed labels Aug 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aarch64 - update EqDbl to use SIMD/FP instructions #7914

Aarch64 - update EqDbl to use SIMD/FP instructions #7914

swalk-cavium commented Jul 12, 2017

swalk-cavium commented Jul 13, 2017

mxw left a comment

mxw Jul 24, 2017

swalk-cavium Jul 24, 2017

mxw Jul 24, 2017

swalk-cavium Jul 24, 2017

swalk-cavium Jul 25, 2017

hhvm-bot commented Jul 24, 2017

jim-saxman commented Jul 25, 2017

hhvm-bot commented Jul 26, 2017

swalk-cavium commented Jul 26, 2017

facebook-github-bot commented Aug 17, 2017

mxw commented Aug 17, 2017

swalk-cavium commented Aug 17, 2017

swalk-cavium commented Aug 17, 2017

dave-estes-QCOM left a comment

dave-estes-QCOM Aug 18, 2017

swalk-cavium Aug 21, 2017

dave-estes-QCOM Aug 18, 2017

dave-estes-QCOM Aug 18, 2017

swalk-cavium commented Aug 18, 2017

swalk-cavium commented Aug 18, 2017

facebook-github-bot commented Aug 23, 2017

facebook-github-bot commented Aug 23, 2017

mxw commented Aug 23, 2017

facebook-github-bot commented Aug 24, 2017

swalk-cavium commented Dec 4, 2017

		@@ -1352,6 +1352,22 @@ void Assembler::frintz(const FPRegister& fd,
		}


		void Assembler::fcmeq(const FPRegister& fd,

		@@ -909,29 +909,17 @@ void Vgen::emit(const unpcklpd& i) {
		///////////////////////////////////////////////////////////////////////////////

		void Vgen::emit(const cmpsd& i) {

Aarch64 - update EqDbl to use SIMD/FP instructions #7914

Are you sure you want to change the base?

Aarch64 - update EqDbl to use SIMD/FP instructions #7914

Conversation

swalk-cavium commented Jul 12, 2017

Before

After

swalk-cavium commented Jul 13, 2017

mxw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhvm-bot commented Jul 24, 2017

jim-saxman commented Jul 25, 2017

hhvm-bot commented Jul 26, 2017

swalk-cavium commented Jul 26, 2017

facebook-github-bot commented Aug 17, 2017

mxw commented Aug 17, 2017

swalk-cavium commented Aug 17, 2017

swalk-cavium commented Aug 17, 2017

dave-estes-QCOM left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swalk-cavium commented Aug 18, 2017

swalk-cavium commented Aug 18, 2017

facebook-github-bot commented Aug 23, 2017

facebook-github-bot commented Aug 23, 2017

mxw commented Aug 23, 2017

facebook-github-bot commented Aug 24, 2017

swalk-cavium commented Dec 4, 2017