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

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


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


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:5145
+  Register DupSrcReg = Dup->getOperand(1).getReg();
+  Dup->getOperand(1).setIsKill(false);
+  MRI.constrainRegClass(DupSrcReg, RC);
----------------
What does machine combiner usually do about killed registers? What happens if there is, for example:
```
  %1 = dup %0
  somethingelse killed %0
  fmul %1
```


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99662