[PATCH] D63619: [AMDGPU] hazard recognizer for fp atomic to s_denorm_mode

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/GCNHazardRecognizer.cpp:148
 
+  if (checkFPAtomicToDenromModeHazard(MI) > 0)
+    return NoopHazard;
----------------
Typo Denrom


================
Comment at: lib/Target/AMDGPU/GCNHazardRecognizer.cpp:1156-1157
+      return false;
+    if (!I->mayLoad() || !I->mayStore() || I->memoperands_empty())
+      return false;
+    const MachineMemOperand *MemOp = *I->memoperands_begin();
----------------
Assuming false for memoperands_empty is wrong


================
Comment at: lib/Target/AMDGPU/GCNHazardRecognizer.cpp:1157-1163
+      return false;
+    const MachineMemOperand *MemOp = *I->memoperands_begin();
+    if (!MemOp->isAtomic())
+      return false;
+    const Value *V = MemOp->getValue();
+    return !V || !V->getType()->isPointerTy() || // assume worst.
+           V->getType()->getPointerElementType()->isFloatTy();
----------------
This whole approach to identifying the atomic opcodes doesn't really work. You can't rely on any of the information in the memoperand, especially the IR pointee type. We do have an SIInstrFlags::maybeAtomic, but I think you need to check the opcode for if it's an FP one


================
Comment at: lib/Target/AMDGPU/GCNHazardRecognizer.cpp:1161-1163
+    const Value *V = MemOp->getValue();
+    return !V || !V->getType()->isPointerTy() || // assume worst.
+           V->getType()->getPointerElementType()->isFloatTy();
----------------
Can't use the pointee type


================
Comment at: test/CodeGen/AMDGPU/fp-atomic-to-s_denormmode.mir:38
+  bb.0:
+    FLAT_ATOMIC_FMIN undef %0:vreg_64, undef %1:vgpr_32, 0, 0, implicit $exec, implicit $flat_scr :: (volatile load store seq_cst seq_cst 4 on `float addrspace(1)* undef`)
+    %2:vgpr_32 = V_ADD_F32_e32 undef %1:vgpr_32, undef %1:vgpr_32, implicit $exec
----------------
Should test all the FP atomic opcodes


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

https://reviews.llvm.org/D63619