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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

The legalizer should also get the code to split this back into the separate opcodes



================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:944
 
+bool CombinerHelper::tryCombineDivRem(MachineInstr &MI) {
+  unsigned Opcode = MI.getOpcode();
----------------
This needs to have some kind of legalization check. We should only do this if either the combined divrem is legal or legalizable, or before legaliation


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:946-948
+  if (Opcode != TargetOpcode::G_SDIV && Opcode != TargetOpcode::G_SREM &&
+      Opcode != TargetOpcode::G_UDIV && Opcode != TargetOpcode::G_UREM)
+    return false;
----------------
This can probably be an assert, the opcode should come from the tablegen opcode list


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:974
+        matchEqualDefs(MI.getOperand(2), UseMI.getOperand(2))) {
+      MachineIRBuilder MIRBuilder(MI);
+      Register DestDivReg, DestRemReg;
----------------
Should not construct single use MIRBuilders. There should be one available in the combiner state


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp:72-76
+  case TargetOpcode::G_SDIV:
+  case TargetOpcode::G_SREM:
+  case TargetOpcode::G_UDIV:
+  case TargetOpcode::G_UREM:
+    return Helper.tryCombineDivRem(MI);
----------------
Should be called by the generic tablegen code in include/llvm/Target/GlobalISel/Combine.td


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/prelegalizer-combiner-divrem.mir:4
+
+---
+name: test_sdiv_srem
----------------
Should add some vector tests. Also cases with other uses, and negative cases with mismatched signed/unsigned pairs


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96013