[PATCH] D137624: [AMDGPU] Declutter applyPreexistingWaitcnt()

[Jay Foad via Phabricator via llvm-commits]

foad accepted this revision.
foad added a comment.
This revision is now accepted and ready to land.

Looks fine with or without nits addressed.



================
Comment at: llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp:848
+  MachineOperand &MO = MI.getOperand(OpIdx);
+  if (NewEnc != MO.getImm()) {
+    MO.setImm(NewEnc);
----------------
Slightly neater to return early if they //are// equal.


================
Comment at: llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp:855
+
+static bool updateNamedOperandIfDifferent(MachineInstr &MI, uint16_t NamedIdx,
+                                          unsigned NewEnc) {
----------------
I know you've copied it from elsewhere but NamedIdx is a terrible name. How about OpName?


================
Comment at: llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp:917
     if (Wait.hasWaitExceptVsCnt()) {
-      unsigned NewEnc = AMDGPU::encodeWaitcnt(IV, Wait);
-      unsigned OldEnc = WaitcntInstr->getOperand(0).getImm();
-      if (OldEnc != NewEnc) {
-        WaitcntInstr->getOperand(0).setImm(NewEnc);
-        Modified = true;
-      }
+      Modified |= updateOperandIfDifferent(*WaitcntInstr, 0,
+                                           AMDGPU::encodeWaitcnt(IV, Wait));
----------------
This could also update the named operand simm16 for consistency. There's no particular rule about whether code like this should look up operand indexes or just hard code them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137624