[PATCH] D136754: [MachineCombiner] Extend reassociation logic to handle inverse instructions

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:1179
+  /// Return true when \P Opcode has inverse operation (e.g. add for sub, mul
+  /// for div). Overriding this method, don't forget to override
+  /// `getInverseOpcode` as well.
----------------
I'm not sure we should mention mul/div here. I don't think you can reassociate them the same way.


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:1181
+  /// `getInverseOpcode` as well.
+  virtual bool hasInverseOpcode(unsigned Opcode) const { return false; }
+
----------------
Can we make getInverseOpcode return an Optional<unsigned> so we can merge `hasInverseOpcode` and `getInverseOpcode`?


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:1193
+  virtual bool
+  isInverseInstAssociativeAndCommutative(const MachineInstr &Inst) const {
+    return false;
----------------
Can we not figure this out using getInverseOpcode?


================
Comment at: llvm/lib/CodeGen/TargetInstrInfo.cpp:837
+  assert(areOpcodesEqualOrInverse(Root.getOpcode(), Prev.getOpcode()) &&
+         "Incorreclty matched pattern");
+  unsigned AssocCommutOpcode = Root.getOpcode();
----------------
Incorrectly*


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136754