[PATCH] D154488: [PowerPC] Define SchedModel for Power8

[Qiu Chaofan via Phabricator via llvm-commits]

qiucf added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCScheduleP8.td:50
+  def P8_FP_Scal : ProcResource<2> { let Super = P8_FPU; }
+  def P8_FP_2x64 : ProcResource<4> { let Super = P8_FPU; }
+  def P8_FP_4x32 : ProcResource<2> { let Super = P8_VMX; }
----------------
shchenz wrote:
> Sorry, this still confuses me. The super is `P8_FPU` and `P8_FPU` has 4 execution units which can be executed as 2 way SIMD operation for double. So why we set the ProcResource value to 4 here?
Thanks, updated.


================
Comment at: llvm/lib/Target/PowerPC/PPCScheduleP8.td:51
+  def P8_FP_2x64 : ProcResource<4> { let Super = P8_FPU; }
+  def P8_FP_4x32 : ProcResource<2> { let Super = P8_VMX; }
 
----------------
shchenz wrote:
> Setting `P8_VMX` as super of "4xSingle" also seems weird. In ISA of pwr8, I think most instructions that handle 4xSingle type are VSX related and from the UM "10.3.2 Instructional Latencies and Throughputs", I saw most of them are using pipeline `FPU`. So I guess, we should use `P8_FPU` as Super instead?
> **2.1.3 Speculative Superscalar Inner Core Organization**:
> 
> - Two VMX execution units capable of executing simple FX, permute, complex FX, and 4-way SIMD single-precision floating-point operations

I think the reason is before VSX, Altivec already had instructions for 4xSingle vectors and they are implemented within VMX units. So their VSX equivalents uses the same execution units.


================
Comment at: llvm/lib/Target/PowerPC/PPCScheduleP8.td:48
+  def P8_FP_1 : ProcResource<2> { let Super = P8_FPU; }
+  def P8_FP_2 : ProcResource<4> { let Super = P8_FPU; }
+  def P8_FP_4 : ProcResource<2> { let Super = P8_VMX; }
----------------
shchenz wrote:
> Why this unit number for type "2xdouble" is set to 4? Its parent `P8_VMX` only has 2.
Its parent `P8_FPU` has 4 units.


================
Comment at: llvm/test/CodeGen/PowerPC/atomics-i16-ldst.ll:184
+; CHECK-PREP10-NEXT:    lbzx r3, r3, r4
+; CHECK-PREP10-NEXT:    blr
 entry:
----------------
shchenz wrote:
> We may need another name for check prefix `PREP10`? And also double check if prefixes `P8` and `P9` are still used in this file?
Like `CHECK-NONP10` or `CHECK-P8P9`? But it actually has the same meaning as `CHECK-PREP10`.


================
Comment at: llvm/test/CodeGen/PowerPC/bool-math.ll:120
 
 define i8 @low_bit_select_constants_bigger_true_same_size_result(i8 %x) {
   %a = and i8 %x, 1
----------------
shchenz wrote:
> Weird. Now there is no check for this function?
Because the test RUN line did not specify `-mcpu`, so that Linux uses pwr8 while AIX uses pwr7. Fixed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154488/new/

https://reviews.llvm.org/D154488