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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1027
+  // matched div/rem pair.
+  if (dominates(MI, *OtherMI))
+    InsertionPoint = &MI;
----------------
I'd rather keep all legality checks in the match function. But also I realized it should work if you check if the parent blocks are the same and then you can assume otherMI is earlier


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1021
+      Opcode == TargetOpcode::G_SDIV || Opcode == TargetOpcode::G_SREM;
+  Builder.setInstrAndDebugLoc(MI);
+  Builder.buildInstr(IsSigned ? TargetOpcode::G_SDIVREM
----------------
cdevadas wrote:
> arsenm wrote:
> > I still suspect something is wrong with the insertion. I don't believe it's guaranteed that the first instruction you find will dominate the second, so you don't know where to place the insert point. 
> The instructions are visited in the top-down order and `MI` will always dominate the `otherMI`. 
The combiner visits bottom up.

Actually if you check that these are in the same block, you can just use OtherMI as the insert point


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96013