[PATCH] D99662: [AArch64] Add Machine InstCombiner patterns for FMUL indexed variant

[Andrew Savonichev via Phabricator via llvm-commits]

asavonic added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:5136
+    MachineRegisterInfo &MRI) {
+  assert(IdxDupOp == 1 || IdxDupOp == 2 && "Invalid index of FMUL operand");
+
----------------
dmgreen wrote:
> This needs some brackets around the || I think.
Thank you! Done.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:5145
+  Register DupSrcReg = Dup->getOperand(1).getReg();
+  Dup->getOperand(1).setIsKill(false);
+  MRI.constrainRegClass(DupSrcReg, RC);
----------------
dmgreen wrote:
> What does machine combiner usually do about killed registers? What happens if there is, for example:
> ```
>   %1 = dup %0
>   somethingelse killed %0
>   fmul %1
> ```
Usually the new instruction is replacing the old one, so the kill state is just copied.
Here we need to extend lifetime of the register, and your example confirms that `setIsKill(0)` is not enough. We can use `MRI.clearKillFlags` instead to remove the kill state from all uses of the register.

Added this case to machine-combiner-fmul-dup.mir.


================
Comment at: llvm/test/CodeGen/AArch64/machine-combiner-fmul-dup.mir:1
+# RUN: llc -run-pass=machine-combiner -o - -simplify-mir -mtriple=aarch64-unknown-linux-gnu -mattr=+fullfp16 %s | FileCheck %s
+--- |
----------------
dmgreen wrote:
> It's probably worth adding verify-machineinstrs to tests like this. There are expensive bots that will check anyway, but it's good to be deliberate on tests like this. I might also use the update_mir_test_checks script, but that's up to you depending on how useful the check lines it adds are.
They can be useful to see details like kill states.
Added them to the test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99662