[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:1193
+  virtual bool
+  isInverseInstAssociativeAndCommutative(const MachineInstr &Inst) const {
+    return false;
----------------
craig.topper wrote:
> 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?
Good suggestion, thanks! I've added  `Invert` bool flag. One thing I'd like to mention explicitly is that I've added the default value to `Invert` argument. In general, it's dangerous in C++ to combine virtual functions and default argument values. However, I can't imagine the situation when a specific target decides to use another default value for the argument as it'll break machine combiner logic. So, exactly in this case I think it's completely safe.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136754