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

[Stephen Thomas via Phabricator via llvm-commits]

stepthomas added inline comments.


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


================
Comment at: llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp:855
+
+static bool updateNamedOperandIfDifferent(MachineInstr &MI, uint16_t NamedIdx,
+                                          unsigned NewEnc) {
----------------
foad wrote:
> I know you've copied it from elsewhere but NamedIdx is a terrible name. How about OpName?
That's certainly no worse, I'll update that too.


================
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));
----------------
foad wrote:
> 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.
That would mean I could just have one "updateIfDifferent()" function, which is neater.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137624