[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 marked 6 inline comments as done and an inline comment as not done.
mingmingl added a comment.

In D130100#3673897 <https://reviews.llvm.org/D130100#3673897>, @dmgreen wrote:

> Do you have details on the motivating case for this? I was under the impression that it was usually handled by ISel, but the tests certainly suggest it isn't always the case. There will be times for DAG ISel which cross basic-blocks, which it won't be able to match. A number of the other test changes look like artifacts from how tablegen patterns are defined.
>
> The AArch64MIPeepholeOptimizer seems to be a rich source of bugs. Every time we make a change in it, it takes several attempt to get it correct.

It's true that most of the cross-register-back copies are introduced in the (multi-step) ISel pass. May I hear some feedback on whether the general idea of combining {ldr + copy} into {ldr} is good to go, given that fixing ISel one-by-one could be laborious.

Also update the implementation by bailing out on illegal conditions



================
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))
----------------
mingmingl wrote:
> 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.
Leaving this open before go/no-go discussion with peephole converges.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130100