[PATCH] D105854: [PowerPC] Inefficient scheduling of ACC registers results in many copies.

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp:483
+
+  bool BaseImplRetVal = TargetRegisterInfo::getRegAllocationHints(
+      VirtReg, Order, Hints, MF, VRM, Matrix);
----------------
I am not 100% positive this is the intent here, but a comment should definitely be added (update my suggestion if it does not adequately capture the intent).
```
// Call the base implementation first to set any hints based on
// the usual heuristics and decide what the return value should
// be. We want to return the same value returned by the base
// implementation as we are not looking to force a specific allocation
// but rather to just provide a hint.
```


================
Comment at: llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp:498
+        Hints.push_back(HintReg);
+        // We don't set BaseImplRetVal here. We only want to consider this
+        // regster first not force the regster allocator to use it.
----------------
I think this comment is more appropriate on the call to the function from the base class (see above).


================
Comment at: llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp:511
+        // UACC register as a hint.
+        if (ACCPhys >= PPC::ACC0 && ACCPhys >= PPC::ACC7) {
+          Register HintReg = PPC::UACC0 + (ACCPhys - PPC::ACC0);
----------------
Is it possible for this to not be true? Should this maybe just be an assert instead?


================
Comment at: llvm/lib/Target/PowerPC/PPCRegisterInfo.h:97
 
+  bool getRegAllocationHints(Register VirtReg, ArrayRef<MCPhysReg> Order,
+                             SmallVectorImpl<MCPhysReg> &Hints,
----------------
```
// Provide hints to the register allocator for allocating subregisters
// of primed and unprimed accumulators. For example, if accumulator
// ACC5 is assigned, we also want to assign UACC5 to the input.
// Similarly if UACC5 is assigned, we want to assign VSRp10, VSRp11
// to its inputs.
```


================
Comment at: llvm/lib/Target/PowerPC/PPCRegisterInfo.td:452
                                                       ACC4, ACC5, ACC6, ACC7)> {
+  let AllocationPriority = 63;
   let Size = 512;
----------------
Please add a comment regarding how the values were selected.


================
Comment at: llvm/test/CodeGen/PowerPC/ppc64-acc-schedule.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+target triple = "powerpc64le-unknown-linux-gnu"
----------------
I think that for this test case, it would be more useful to see how the code generation changes with this patch. Please pre-commit it with current code generation and then this review will show the difference.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105854