[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:1193
+  virtual bool
+  isInverseInstAssociativeAndCommutative(const MachineInstr &Inst) const {
+    return false;
----------------
asi-sc wrote:
> asi-sc wrote:
> > 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.
> I left this unchanged for now and uploaded a patch that shows how it looks like in the current design D138660 . If there is still a desire to merge `isInverseInstAssociativeAndCommutative` with `isAssociativeAndCommutative`, then we should change the latter to take not only the instruction, but the opcode as well
> ```
> /// Return true when \P Inst with \P Opcode is both associative and commutative.
> virtual bool isAssociativeAndCommutative(const MachineInstr &Inst, unsinged Opcode) const;
> ```
> which seems to me pretty unclear from the user's point of view of this interface.
Nevermind. My suggestion. I realize now that it requires the instruction to exist with the inverted opcode.

Would it make sense to pass an `Invert` bool to isAssociativeAndCommutative so we don't need to two interfaces?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136754