[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.
----------------
asi-sc wrote:
> 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.
Disregard my previous comment. I agree we should not mention mul/div here as we cannot guarantee transformation safety.


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:1181
+  /// `getInverseOpcode` as well.
+  virtual bool hasInverseOpcode(unsigned Opcode) const { return false; }
+
----------------
asi-sc wrote:
> 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.
Thanks, done.


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:1193
+  virtual bool
+  isInverseInstAssociativeAndCommutative(const MachineInstr &Inst) const {
+    return false;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136754