[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 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.

This is a good point. I understand that DAG ISel is probably a better place to handle this combine, to avoid fiddling with various kinds of load/store operations.

As shown in the `.mir` test case, the machine instructions are from the same basic block (not cross basic blocks, thereby code sinking won't help)

Existing pattern <https://github.com/llvm/llvm-project/blob/80cb0cab4e86e149d06add40ee39e9b6430a801b/llvm/lib/Target/AArch64/AArch64InstrInfo.td#L5119> in `AArch64InstrInfo.td` doesn't catch this;

Debug log of instruction-selection shows it's very likely due to lack of type legalization (from int64 to fp). I'll look into the details and see what's missing.

> 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.

Thanks for pointing out a different place to solve the same problem. I share the concern that general memory operations are hard to rewrite correctly, and even if they are correct, it's better to make them happen in the pipeline.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130100