[PATCH] D95432: AMDGPU/GlobalISel: Combine uniform icmp

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegBankCombiner.cpp:72
+  if (!isSgprRegBank(Condition) ||
+      MatchInfo.ICmp->getOpcode() != TargetOpcode::G_ICMP)
+    return false;
----------------
Opcode check first?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegBankCombiner.cpp:92
+      for (auto &TruncUse : MRI.use_nodbg_instructions(TruncDef)) {
+        if (!Helper.dominates(MI, TruncUse))
+          return false;
----------------
You shouldn't need to check dominance (and I don't see how this would ever not be the case)


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegBankCombiner.cpp:103
+    MachineInstr &MI, MoveUniformICmpMatchInfo &MatchInfo) {
+  MatchInfo.ICmp->moveBefore(&MI);
+  if (MatchInfo.MoveTrunc)
----------------
arsenm wrote:
> This really shouldn't be trying to move instructions. Do you even really need to erase the trunc? If it's dead it will be removed already
This isn't changing the operands anymore? (I also think just creating the new instruction with the new operand is cleaner than modifying in place, doing it that way should fix the multiple use case too)


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegBankCombiner.cpp:103-107
+  MatchInfo.ICmp->moveBefore(&MI);
+  if (MatchInfo.MoveTrunc)
+    MatchInfo.Trunc->moveBefore(&MI);
+  else
+    MatchInfo.Trunc->eraseFromParent();
----------------
This really shouldn't be trying to move instructions. Do you even really need to erase the trunc? If it's dead it will be removed already


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

https://reviews.llvm.org/D95432