AArch64 Cortex-A57 FDIV/FSQRT scheduling fix (W-unit)

[andrew.zhogin]

Summary

According to Cortex-A57 doc (http://infocenter.arm.com/help/topic/com.arm.doc.uan0015b/Cortex_A57_Software_Optimization_Guide_external.pdf), FDIV/FSQRT instructions should use F0 unit (W-unit in AArch64SchedA57.td, the same as cryptography instructions), not F1 unit (X-unit in td, like ASIMD absolute diff accum SABA/UABA).

This patch changes FDIV/FSQRT scheduling declarations to use A57UnitW instead of A57UnitX. Also, latencies for those instructions are corrected.

[rovka]

test/CodeGen/AArch64/arm64-misched-A57-div-fix.ll

6

Tests shouldn't rely on grep, please use FileCheck instead.
http://llvm.org/docs/TestingGuide.html#writing-new-regression-tests

[andrew.zhogin]

FileCheck instead of grep in the regression test.

[rovka]

I don't see anything wrong with this, it matches my reading of the manual. Do you have any performance numbers to share? (e.g. test-suite results)

[andrew.zhogin]

In D27521#624936, @rovka wrote:

I don't see anything wrong with this, it matches my reading of the manual. Do you have any performance numbers to share? (e.g. test-suite results)

No, don't have any.

[rengolin]

Can you run any benchmark and share relative numbers? Despite the fact that the change is obvious, you may uncover pre-existing bugs in the compiler that need addressing before we can change this. The LLVM test-suite, SPEC, EEMBC, anything would help to have an idea.

cheers,
--renato

test/CodeGen/AArch64/arm64-misched-A57-div-fix.ll

6

It should also check for the unit you want to appear, not just the one you don't. :)

[andrew.zhogin]

Test corrections: added check for right scheduling unit and latency.

[andrew.zhogin]

In D27521#625100, @rengolin wrote:

Can you run any benchmark and share relative numbers? Despite the fact that the change is obvious, you may uncover pre-existing bugs in the compiler that need addressing before we can change this. The LLVM test-suite, SPEC, EEMBC, anything would help to have an idea.

I don't have access to Cortex-A57 system.
I did only check-all runs at my X86-64 desktop (green).

[rengolin]

In D27521#627385, @andrew.zhogin wrote:

I don't have access to Cortex-A57 system.
I did only check-all runs at my X86-64 desktop (green).

Unfortunately, check-all will make no difference here. You need benchmarks, not conformance tests.

I can run these for you, but it'll take a while, since we're moving labs and my benchmarking machine is being upgraded at the moment. We can probably have something for beginning next year, unless someone else can do that quicker.

cheers,
--renato

[kristof.beyls]

In D27521#627385, @andrew.zhogin wrote:

In D27521#625100, @rengolin wrote:

Can you run any benchmark and share relative numbers? Despite the fact that the change is obvious, you may uncover pre-existing bugs in the compiler that need addressing before we can change this. The LLVM test-suite, SPEC, EEMBC, anything would help to have an idea.

I don't have access to Cortex-A57 system.
I did only check-all runs at my X86-64 desktop (green).

I've run this patch on top of r290135 on test-suite, spec2K, spec2K6 and a few internal benchmarks.
I saw 2 minor regressions and 2 bigger improvements on internal benchmarks.
Across test-suite, spec2K and spec2K6, all results were within noise levels.

[rengolin]

Thanks Kristof,

I think that address my worries. :)

LGTM, thanks!

[andrew.zhogin]

In D27521#628679, @rengolin wrote:

Thanks Kristof,

I think that address my worries. :)

LGTM, thanks!

Thanks for the review.

I don't have commit access so could you please commit the patch?

[rengolin]

Committed in r290426. Cheers!

lib/Target/AArch64/AArch64SchedA57.td

 def : SchedAlias<WriteLDIdx, A57Write_4cyc_1I_1L>;
 def : SchedAlias<WriteSTIdx, A57Write_1cyc_1I_1S>;
 def : SchedAlias<WriteF,     A57Write_3cyc_1V>;
 def : SchedAlias<WriteFCmp,  A57Write_3cyc_1V>;
 def : SchedAlias<WriteFCvt,  A57Write_5cyc_1V>;
 def : SchedAlias<WriteFCopy, A57Write_5cyc_1L>;
 def : SchedAlias<WriteFImm,  A57Write_3cyc_1V>;
 def : SchedAlias<WriteFMul,  A57Write_5cyc_1V>;
-def : SchedAlias<WriteFDiv,  A57Write_18cyc_1X>;
+def : SchedAlias<WriteFDiv,  A57Write_17cyc_1W>;
 def : SchedAlias<WriteV,     A57Write_3cyc_1V>;
 def : SchedAlias<WriteVLD,   A57Write_5cyc_1L>;
 def : SchedAlias<WriteVST,   A57Write_1cyc_1S>;
 
 def : WriteRes<WriteAtomic,  []> { let Unsupported = 1; }
 
 def : WriteRes<WriteSys,     []> { let Latency = 1; }
 def : WriteRes<WriteBarrier, []> { let Latency = 1; }

 // ASIMD FP convert, long and narrow
 def : InstRW<[A57Write_8cyc_3V], (instregex "^FCVT(L|N|XN)v")>;
 // ASIMD FP convert, other, D-form
 def : InstRW<[A57Write_5cyc_1V], (instregex "^[FVSU]CVT([AMNPZ][SU])?(_Int)?(v2f32|v1i32|v2i32|v1i64)")>;
 // ASIMD FP convert, other, Q-form
 def : InstRW<[A57Write_5cyc_2V], (instregex "^[FVSU]CVT([AMNPZ][SU])?(_Int)?(v4f32|v2f64|v4i32|v2i64)")>;
 
 // ASIMD FP divide, D-form, F32
-def : InstRW<[A57Write_18cyc_1X], (instregex "FDIVv2f32")>;
+def : InstRW<[A57Write_17cyc_1W], (instregex "FDIVv2f32")>;
 // ASIMD FP divide, Q-form, F32
-def : InstRW<[A57Write_36cyc_2X], (instregex "FDIVv4f32")>;
+def : InstRW<[A57Write_34cyc_2W], (instregex "FDIVv4f32")>;
 // ASIMD FP divide, Q-form, F64
-def : InstRW<[A57Write_64cyc_2X], (instregex "FDIVv2f64")>;
+def : InstRW<[A57Write_64cyc_2W], (instregex "FDIVv2f64")>;
 
 // Note: These were simply duplicated from ASIMD FDIV because of missing documentation
 // ASIMD FP square root, D-form, F32
-def : InstRW<[A57Write_18cyc_1X], (instregex "FSQRTv2f32")>;
+def : InstRW<[A57Write_17cyc_1W], (instregex "FSQRTv2f32")>;
 // ASIMD FP square root, Q-form, F32
-def : InstRW<[A57Write_36cyc_2X], (instregex "FSQRTv4f32")>;
+def : InstRW<[A57Write_34cyc_2W], (instregex "FSQRTv4f32")>;
 // ASIMD FP square root, Q-form, F64
-def : InstRW<[A57Write_64cyc_2X], (instregex "FSQRTv2f64")>;
+def : InstRW<[A57Write_64cyc_2W], (instregex "FSQRTv2f64")>;
 
 // ASIMD FP max/min, normal, D-form
 def : InstRW<[A57Write_5cyc_1V], (instregex "^(FMAX|FMIN)(NM)?(v2f32)")>;
 // ASIMD FP max/min, normal, Q-form
 def : InstRW<[A57Write_5cyc_2V], (instregex "^(FMAX|FMIN)(NM)?(v4f32|v2f64)")>;
 // ASIMD FP max/min, pairwise, D-form
 def : InstRW<[A57Write_5cyc_1V], (instregex "^(FMAX|FMIN)(NM)?P(v2f32|v2i32)")>;
 // ASIMD FP max/min, pairwise, Q-form

 def A57WriteFPMA  : SchedWriteRes<[A57UnitV]> { let Latency = 9;  }
 def A57ReadFPMA5  : SchedReadAdvance<5, [A57WriteFPMA]>;
 def A57ReadFPM    : SchedReadAdvance<0>;
 def : InstRW<[A57WriteFPMA, A57ReadFPM, A57ReadFPM, A57ReadFPMA5], (instregex "^FN?M(ADD|SUB)[DS]rrr")>;
 
 def : InstRW<[A57Write_10cyc_1L_1V], (instregex "^[FSU]CVT[AMNPZ][SU](_Int)?[SU]?[XW]?[DS]?[rds]i?")>;
 def : InstRW<[A57Write_10cyc_1L_1V], (instregex "^[SU]CVTF")>;
 
-def : InstRW<[A57Write_32cyc_1X], (instrs FDIVDrr)>;
-def : InstRW<[A57Write_18cyc_1X], (instrs FDIVSrr)>;
+def : InstRW<[A57Write_32cyc_1W], (instrs FDIVDrr)>;
+def : InstRW<[A57Write_17cyc_1W], (instrs FDIVSrr)>;
 
 def : InstRW<[A57Write_5cyc_1V], (instregex "^F(MAX|MIN).+rr")>;
 
 def : InstRW<[A57Write_5cyc_1V], (instregex "^FRINT.+r")>;
 
-def : InstRW<[A57Write_32cyc_1X], (instrs FSQRTDr)>;
-def : InstRW<[A57Write_18cyc_1X], (instrs FSQRTSr)>;
+def : InstRW<[A57Write_32cyc_1W], (instrs FSQRTDr)>;
+def : InstRW<[A57Write_17cyc_1W], (instrs FSQRTSr)>;
 
 def : InstRW<[A57Write_5cyc_1L, WriteLDHi], (instrs LDNPDi)>;
 def : InstRW<[A57Write_6cyc_2L, WriteLDHi], (instrs LDNPQi)>;
 def : InstRW<[A57Write_5cyc_1L, WriteLDHi], (instrs LDNPSi)>;
 def : InstRW<[A57Write_5cyc_1L, WriteLDHi], (instrs LDPDi)>;
 def : InstRW<[A57Write_5cyc_1L, WriteLDHi, WriteAdr], (instrs LDPDpost)>;
 def : InstRW<[A57Write_5cyc_1L, WriteLDHi, WriteAdr], (instrs LDPDpre)>;
 def : InstRW<[A57Write_6cyc_2L, WriteLDHi], (instrs LDPQi)>;

lib/Target/AArch64/AArch64SchedA57WriteRes.td

 //===----------------------------------------------------------------------===//
 // Define Generic 1 micro-op types
 
 def A57Write_5cyc_1L  : SchedWriteRes<[A57UnitL]> { let Latency = 5;  }
 def A57Write_5cyc_1M  : SchedWriteRes<[A57UnitM]> { let Latency = 5;  }
 def A57Write_5cyc_1V  : SchedWriteRes<[A57UnitV]> { let Latency = 5;  }
 def A57Write_5cyc_1W  : SchedWriteRes<[A57UnitW]> { let Latency = 5;  }
 def A57Write_10cyc_1V : SchedWriteRes<[A57UnitV]> { let Latency = 10; }
-def A57Write_18cyc_1X : SchedWriteRes<[A57UnitX]> { let Latency = 18;
-                                                    let ResourceCycles = [18]; }
+def A57Write_17cyc_1W : SchedWriteRes<[A57UnitW]> { let Latency = 17;
+                                                    let ResourceCycles = [17]; }
 def A57Write_19cyc_1M : SchedWriteRes<[A57UnitM]> { let Latency = 19;
                                                     let ResourceCycles = [19]; }
 def A57Write_1cyc_1B  : SchedWriteRes<[A57UnitB]> { let Latency = 1;  }
 def A57Write_1cyc_1I  : SchedWriteRes<[A57UnitI]> { let Latency = 1;  }
 def A57Write_1cyc_1S  : SchedWriteRes<[A57UnitS]> { let Latency = 1;  }
 def A57Write_2cyc_1M  : SchedWriteRes<[A57UnitM]> { let Latency = 2;  }
-def A57Write_32cyc_1X : SchedWriteRes<[A57UnitX]> { let Latency = 32;
+def A57Write_32cyc_1W : SchedWriteRes<[A57UnitW]> { let Latency = 32;
                                                     let ResourceCycles = [32]; }
 def A57Write_35cyc_1M : SchedWriteRes<[A57UnitM]> { let Latency = 35;
                                                     let ResourceCycles = [35]; }
 def A57Write_3cyc_1M  : SchedWriteRes<[A57UnitM]> { let Latency = 3;  }
 def A57Write_3cyc_1V  : SchedWriteRes<[A57UnitV]> { let Latency = 3;  }
 def A57Write_3cyc_1W  : SchedWriteRes<[A57UnitW]> { let Latency = 3;  }
 def A57Write_3cyc_1X  : SchedWriteRes<[A57UnitX]> { let Latency = 3;  }
 def A57Write_4cyc_1L  : SchedWriteRes<[A57UnitL]> { let Latency = 4;  }
 def A57Write_4cyc_1X  : SchedWriteRes<[A57UnitX]> { let Latency = 4;  }
 def A57Write_9cyc_1V  : SchedWriteRes<[A57UnitV]> { let Latency = 9;  }
 def A57Write_6cyc_1M  : SchedWriteRes<[A57UnitM]> { let Latency = 6;  }
 def A57Write_6cyc_1V  : SchedWriteRes<[A57UnitV]> { let Latency = 6;  }
 
 
 //===----------------------------------------------------------------------===//
 // Define Generic 2 micro-op types
 
-def A57Write_64cyc_2X    : SchedWriteRes<[A57UnitX, A57UnitX]> {
+def A57Write_64cyc_2W    : SchedWriteRes<[A57UnitW, A57UnitW]> {
   let Latency     = 64;
   let NumMicroOps = 2;
   let ResourceCycles = [32, 32];
 }
 def A57Write_6cyc_1I_1L  : SchedWriteRes<[A57UnitI,
                                           A57UnitL]> {
   let Latency     = 6;
   let NumMicroOps = 2;

 def A57Write_2cyc_2S     : SchedWriteRes<[A57UnitS, A57UnitS]> {
   let Latency     = 2;
   let NumMicroOps = 2;
 }
 def A57Write_2cyc_2V     : SchedWriteRes<[A57UnitV, A57UnitV]> {
   let Latency     = 2;
   let NumMicroOps = 2;
 }
-def A57Write_36cyc_2X    : SchedWriteRes<[A57UnitX, A57UnitX]> {
-  let Latency     = 36;
+def A57Write_34cyc_2W    : SchedWriteRes<[A57UnitW, A57UnitW]> {
+  let Latency     = 34;
   let NumMicroOps = 2;
-  let ResourceCycles = [18, 18];
+  let ResourceCycles = [17, 17];
 }
 def A57Write_3cyc_1I_1M  : SchedWriteRes<[A57UnitI,
                                           A57UnitM]> {
   let Latency     = 3;
   let NumMicroOps = 2;
 }
 def A57Write_3cyc_1I_1S  : SchedWriteRes<[A57UnitI,
                                           A57UnitS]> {

test/CodeGen/AArch64/arm64-misched-A57-div-fix.ll

+; REQUIRES: asserts
+;
+; FDIV/FSQRT should use A57UnitW, not A57UnitX
+;
+; RUN: llc < %s -mtriple=arm64-linux-gnu -mcpu=cortex-a57 -enable-misched -verify-misched -debug-only=misched -o - 2>&1 > /dev/null | FileCheck %s
+;
+; CHECK-NOT: A57UnitX
+; CHECK:      picking next node
+; CHECK:      FDIVSrr
+; CHECK:      Critical: [[NUM:[0-9]+]]c, [[NUM]] A57UnitW
+; CHECK-NEXT: ExpectedLatency: 18c
+
+define float @main(float %a, float %b) nounwind {
+entry:
+  %div = fdiv float %a, %b
+  ret float %div
+}
+