[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
llvm-commits at lists.llvm.org
Fri Jul 22 22:14:36 PDT 2022
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
More information about the llvm-commits
mailing list