[PATCH] D96013: GlobalISel: Try to combine G_[SU]DIV and G_[SU]REM

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:946
+  unsigned Opcode = MI.getOpcode();
+  if (Opcode != TargetOpcode::G_SDIV && Opcode != TargetOpcode::G_SREM &&
+      Opcode != TargetOpcode::G_UDIV && Opcode != TargetOpcode::G_UREM)
----------------
arsenm wrote:
> This can probably be an assert, the opcode should come from the tablegen opcode list
Maybe something like this?

```
switch(Opcode) {
  default:
     llvm_unreachable("Unexpected opcode!");
  case TargetOpcode::G_SDIV:
  case TargetOpcode::G_UDIV:
      IsSigned = Opcode == TargetOpcode::G_SDIV;
      IsDiv = true;
      break;
  case TargetOpcode::G_SREM:
  case TargetOpcode::G_UREM:
     ...
}
```


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:955
+  Register Src2 = MI.getOperand(2).getReg();
+  unsigned DivOpcode = IsSigned ? TargetOpcode::G_SDIV : TargetOpcode::G_UDIV;
+  unsigned RemOpcode = IsSigned ? TargetOpcode::G_SREM : TargetOpcode::G_UREM;
----------------
Why not use `Opcode`? If it's a div, then `Opcode` == `DivOpcode`. If it's a rem, then `Opcode == RemOpcode`.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:977
+      if (IsDiv) {
+        DestDivReg = MI.getOperand(0).getReg();
+        DestRemReg = UseMI.getOperand(0).getReg();
----------------
This part should go in a `MatchInfo` parameter like in the other combines. That should be passed along to an `apply` function which should perform the actual combine.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:984
+
+      MIRBuilder.buildInstr(IsSigned ? TargetOpcode::G_SDIVREM
+                                     : TargetOpcode::G_UDIVREM,
----------------
I think this should go in an `applyCombineDivRem` function like the other combines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96013