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

[Mingming Liu via Phabricator via llvm-commits]

mingmingl added inline comments.


================
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))
----------------
kazu wrote:
> 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.
I didn't see them being triggered from the affected test case from a cursory look (i.e., most affected load operations in the affected test case come without indices)

It definitely makes sense to add the test coverage, even if the fix would be in another place eventually.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130100