[PATCH] D136754: [MachineCombiner] Extend reassociation logic to handle inverse instructions
Anton Sidorenko via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 24 05:08:36 PST 2022
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
More information about the llvm-commits
mailing list