[PATCH] D130100: [AArch64] Combine a load into GPR followed by a copy to FPR to a load into FPR directly through MIPeepholeOpt

[Kazu Hirata via Phabricator via llvm-commits]

kazu added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp:108
+  bool hasSameNumberOfBits(const TargetRegisterClass *FPRegClass,
+                           const TargetRegisterClass *GPRRegClass);
+  bool isGPRegister(const TargetRegisterClass *RC);
----------------
Declare this with `const`.  Alternatively, turn the definition into a function in the anonymous namespace outside the class.


================
Comment at: llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp:109
+                           const TargetRegisterClass *GPRRegClass);
+  bool isGPRegister(const TargetRegisterClass *RC);
+  bool isFPRRegister(const TargetRegisterClass *RC);
----------------
Declare this with `const`.  Alternatively, turn the definition into a function in the anonymous namespace outside the class.


================
Comment at: llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp:110
+  bool isGPRegister(const TargetRegisterClass *RC);
+  bool isFPRRegister(const TargetRegisterClass *RC);
   bool runOnMachineFunction(MachineFunction &MF) override;
----------------
Declare this with `const`.  Alternatively, turn the definition into a function in the anonymous namespace outside the class.


================
Comment at: llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp:222
+bool AArch64MIPeepholeOpt::visitCopy(MachineInstr &MI) {
+  // Transformation before:
+  //
----------------
`Transformation before` sounds a bit strange.  I'd suggest `Before transformation` or simply`Before`.


================
Comment at: llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp:227
+  //
+  // Transformation after:
+  // %3:fpr64 = LDRDui %1:gpr64common, 1 :: (load (s64) from %ir.3)
----------------
Likewise.


================
Comment at: llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp:238
+
+  if (SrcOperand.isReg() && MRI->hasOneNonDBGUse(SrcOperand.getReg())) {
+    Register SrcReg = SrcOperand.getReg();
----------------
Reduce indentation with an early return like so:

```
  if (!SrcOperand.isReg() || !MRI->hasOneNonDBGUse(SrcOperand.getReg()))
    false;
```



================
Comment at: llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp:251-252
+    const TargetRegisterClass *SrcRegClass = MRI->getRegClass(SrcReg);
+    if (isFPRRegister(DstRegClass) && isGPRegister(SrcRegClass) &&
+        hasSameNumberOfBits(DstRegClass, SrcRegClass)) {
+      switch (SrcMI->getOpcode()) {
----------------
Reduce indentation with an early return like so:

```
    if (!isFPRRegister(DstRegClass) || !isGPRegister(SrcRegClass) ||
        !hasSameNumberOfBits(DstRegClass, SrcRegClass))
      return false;
```



================
Comment at: llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp:286
+
+      Register DstReg = DstOperand.getReg();
+      Register ReplacedReg;
----------------
Remove this as you have an identical line about 50 lines above.


================
Comment at: llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp:302-328
+      case AArch64::LDRDroW:
+      case AArch64::LDRDroX:
+      case AArch64::LDRSroW:
+      case AArch64::LDRSroX: {
+        BuildMI(*MI.getParent(), MI, MI.getDebugLoc(), TII->get(NewOpCode),
+                DstOperand.getReg())
+            .add(SrcMI->getOperand(1))
----------------
Do these two blocks of code trigger at all?  I tried `ninja check-llvm` with `assert(0)` placed in these two blocks, but neither one triggered.  It may be tedious to have a test case for every case in the first `switch` statement, but I'd suggest at least one test case for each block in the second `switch` statement -- partly for correctness and partly for coverage.


================
Comment at: llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp:332
+
+      if (NewOpCode != -1) {
+        assert(ReplacedReg.isValid() &&
----------------
Reduce indentation with an early return like so:

```
  if (NewOpCode == -1)
    return false;
```

You might consider placing this check between the two `switch` statements because the second `switch` statement doesn't change `NewOpCode`.


================
Comment at: llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp:335
+               "ReplacedReg expected to be valid when NewOpCode is chosen");
+        // Replace 'ReplacedReg' with 'DstReg', this is mainly to update all
+        // debug uses of 'ReplacedReg' to the correct value.
----------------
Remove `this is` to fix the comma splice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130100