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

[Anton Sidorenko via Phabricator via llvm-commits]

asi-sc 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.
----------------
craig.topper wrote:
> I'm not sure we should mention mul/div here. I don't think you can reassociate them the same way.
Do you mean division by zero cases, e.g. `(A / X) * 0  --> A / (X / 0)` ? We must not introduce new divisors and then it will be legal. Am I missing something? 
If we don't have any other problems with mul/div, I'll fix reassociation patterns.


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:1181
+  /// `getInverseOpcode` as well.
+  virtual bool hasInverseOpcode(unsigned Opcode) const { return false; }
+
----------------
craig.topper wrote:
> Can we make getInverseOpcode return an Optional<unsigned> so we can merge `hasInverseOpcode` and `getInverseOpcode`?
Yeah, I'll do that. My original implementation used Optional, but then I decided that two functions were clearer solution for the interface. However, now I see that having Optional is better than these ugly reminders in the comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:1193
+  virtual bool
+  isInverseInstAssociativeAndCommutative(const MachineInstr &Inst) const {
+    return false;
----------------
craig.topper wrote:
> Can we not figure this out using getInverseOpcode?
Do you suggest changing `isAssociativeAndCommutative` to take not only an instruction, but an opcode as well? This seems to spoil the readability of its interface. However, this will simplify the implementation.


================
Comment at: llvm/lib/CodeGen/TargetInstrInfo.cpp:859
+  //   Y + (A - X) => (Y - X) + A
+  //   Y - (A - X) => (Y - X) - A
+  // REASSOC_XA_YB:
----------------
This is incorrect. Must be `Y - (A - X) => (Y + X) - A`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136754