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

[Petar Avramovic via Phabricator via llvm-commits]

Petar.Avramovic added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegBankCombiner.cpp:92
+      for (auto &TruncUse : MRI.use_nodbg_instructions(TruncDef)) {
+        if (!Helper.dominates(MI, TruncUse))
+          return false;
----------------
arsenm wrote:
> You shouldn't need to check dominance (and I don't see how this would ever not be the case)
Trunc could have a use before the MI so we cant move icmp and trunc past that use.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegBankCombiner.cpp:103
+    MachineInstr &MI, MoveUniformICmpMatchInfo &MatchInfo) {
+  MatchInfo.ICmp->moveBefore(&MI);
+  if (MatchInfo.MoveTrunc)
----------------
arsenm wrote:
> 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)
zext_trunc_fold changes operands. This now only moves icmp (and trunc for multiple use).


================
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();
----------------
Petar.Avramovic wrote:
> arsenm wrote:
> > 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)
> zext_trunc_fold changes operands. This now only moves icmp (and trunc for multiple use).
>Do you even really need to erase the trunc?
ICmp move breaks ssa in mir, trunc uses icmp before it is defined. I meant to delete trunc because this is the place we broke ssa, and we are aware of it. Leaving it to be deleted by something that eliminates dead instructions should work fine (I don't expect anything else to check where uses of this trunc are defined).

>This really shouldn't be trying to move instructions.
What do you suggest, making new icmp (and trunc) or something else?


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/combine-move-uniform-icmp.mir:203
+    %15:sgpr(s1) = G_CONSTANT i1 true
+    %18:sgpr(s32) = G_ANYEXT %11(s1)
+    %19:sgpr(s32) = G_ANYEXT %15(s1)
----------------
Trunc use before select.


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/combine-move-uniform-icmp.mir:213
+    %22:sgpr(s32) = G_ZEXT %11(s1)
+    %14:sgpr(s32) = G_SELECT %22(s32), %1, %13
+    %23:vgpr(s32) = COPY %14(s32)
----------------
After zext_trunc fold this select uses `%17 G_ICMP`  instead of `%22 G_ZEXT` but we can't move icmp because `%11:sgpr(s1) = G_TRUNC %17(s32)` uses icmp and `%18:sgpr(s32) = G_ANYEXT %11(s1)` above uses trunc.


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

https://reviews.llvm.org/D95432