[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:103-107
+  MatchInfo.ICmp->moveBefore(&MI);
+  if (MatchInfo.MoveTrunc)
+    MatchInfo.Trunc->moveBefore(&MI);
+  else
+    MatchInfo.Trunc->eraseFromParent();
----------------
Petar.Avramovic wrote:
> 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?
Yes, if you recreate the desired instruction it should automatically CSE as you need by the builder


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

https://reviews.llvm.org/D95432