AMDGPU: Restore atomic fp feature on FP atomic instruction definitions

[arsenm]

Summary

9931b1f7a4785b6a17fb87b81a3546d61d0cbca1 switched this to checking for
the two specific subtargets, instead of the dedicated feature. This
broke supporting functions which force added the feature when emitting
targets that do not actually support them. This stil does not work for
the targets that use the gfx6/7 or gfx10 encodings.

[rampitec]

We should not produce this for gfx803.

[arsenm]

In D100236#2680833, @rampitec wrote:

We should not produce this for gfx803.

We need to produce something. The expectation is the function should be dynamically dead.

[rampitec]

In D100236#2680838, @arsenm wrote:

In D100236#2680833, @rampitec wrote:

We should not produce this for gfx803.

We need to produce something. The expectation is the function should be dynamically dead.

The feature itself is not sufficient, you cannot just expand atomicrmw into this without knowledge of other target properties. Unfortunately these has changed since first defined.

[arsenm]

In D100236#2680845, @rampitec wrote:

In D100236#2680838, @arsenm wrote:

In D100236#2680833, @rampitec wrote:

We should not produce this for gfx803.

We need to produce something. The expectation is the function should be dynamically dead.

The feature itself is not sufficient, you cannot just expand atomicrmw into this without knowledge of other target properties. Unfortunately these has changed since first defined.

What other target properties? This is supposed to be the target property to say the instruction is available

[rampitec]

In D100236#2706898, @arsenm wrote:

In D100236#2680845, @rampitec wrote:

In D100236#2680838, @arsenm wrote:

In D100236#2680833, @rampitec wrote:

We should not produce this for gfx803.

We need to produce something. The expectation is the function should be dynamically dead.

The feature itself is not sufficient, you cannot just expand atomicrmw into this without knowledge of other target properties. Unfortunately these has changed since first defined.

What other target properties? This is supposed to be the target property to say the instruction is available

All that interactions with ret/noret flavors and flushing. I am not sure how could you produce this instruction for an unrelated HW. I would say atomicrmw shall be expanded into a CAS loop in this case, at least it will work.

[arsenm]

In D100236#2709759, @rampitec wrote:

In D100236#2706898, @arsenm wrote:

In D100236#2680845, @rampitec wrote:

In D100236#2680838, @arsenm wrote:

In D100236#2680833, @rampitec wrote:

We should not produce this for gfx803.

We need to produce something. The expectation is the function should be dynamically dead.

The feature itself is not sufficient, you cannot just expand atomicrmw into this without knowledge of other target properties. Unfortunately these has changed since first defined.

What other target properties? This is supposed to be the target property to say the instruction is available

All that interactions with ret/noret flavors and flushing. I am not sure how could you produce this instruction for an unrelated HW. I would say atomicrmw shall be expanded into a CAS loop in this case, at least it will work.

The point is there is no hardware. This is driven purely by the feature, not the specific device. This is the only feature required to handle this case (plus the mode settings for unsafe/flush).

[rampitec]

OK, I guess it is non-functional anyway.

[arsenm]

ed633a1daad97044063934cf28985491fbed7b4c

llvm/lib/Target/AMDGPU/FLATInstructions.td

 defm SCRATCH_LOAD_SHORT_D16_HI  : FLAT_Real_AllAddr_vi <0x25>;
 defm SCRATCH_STORE_SHORT        : FLAT_Real_AllAddr_vi <0x1a>;
 defm SCRATCH_STORE_SHORT_D16_HI : FLAT_Real_AllAddr_vi <0x1b>;
 defm SCRATCH_STORE_DWORD        : FLAT_Real_AllAddr_vi <0x1c>;
 defm SCRATCH_STORE_DWORDX2      : FLAT_Real_AllAddr_vi <0x1d>;
 defm SCRATCH_STORE_DWORDX3      : FLAT_Real_AllAddr_vi <0x1e>;
 defm SCRATCH_STORE_DWORDX4      : FLAT_Real_AllAddr_vi <0x1f>;
 
-let SubtargetPredicate = isGFX908orGFX90A in {
+let SubtargetPredicate = HasAtomicFaddInsts in {
 defm GLOBAL_ATOMIC_ADD_F32    : FLAT_Global_Real_Atomics_vi <0x04d, 0>;
 defm GLOBAL_ATOMIC_PK_ADD_F16 : FLAT_Global_Real_Atomics_vi <0x04e, 0>;
 }
 
 let SubtargetPredicate = isGFX90AOnly in {
   defm FLAT_ATOMIC_ADD_F64   : FLAT_Real_Atomics_vi<0x4f, FLAT_ATOMIC_ADD_F64, 0>;
   defm FLAT_ATOMIC_MIN_F64   : FLAT_Real_Atomics_vi<0x50, FLAT_ATOMIC_MIN_F64, 0>;
   defm FLAT_ATOMIC_MAX_F64   : FLAT_Real_Atomics_vi<0x51, FLAT_ATOMIC_MAX_F64, 0>;

llvm/test/CodeGen/AMDGPU/global-atomics-fp-wrong-subtarget.ll

+; RUN: llc -march=amdgcn -mcpu=gfx803 < %s | FileCheck -enable-var-scope -check-prefix=GCN %s
+; RUN: llc -march=amdgcn -mcpu=gfx803 -filetype=obj < %s | llvm-objdump --triple=amdgcn--amdhsa --mcpu=gfx803 -d - | FileCheck -check-prefix=DISASSEMBLY-VI %s
+
+; Make sure we can encode and don't fail on functions which have
+; instructions not actually supported by the subtarget.
+; FIXME: This will still fail for gfx6/7 and gfx10 subtargets.
+
+; DISASSEMBLY-VI: .long 0xdd348000                                           // {{[0-9]+}}: DD348000
+; DISASSEMBLY-VI-NEXT: v_cndmask_b32_e32 v0, v0, v0, vcc                     // {{[0-9]+}}: 00000100
+
+define amdgpu_kernel void @global_atomic_fadd_noret_f32_wrong_subtarget(float addrspace(1)* %ptr) #0 {
+; GCN-LABEL: global_atomic_fadd_noret_f32_wrong_subtarget:
+; GCN:       ; %bb.0:
+; GCN-NEXT:    s_load_dwordx2 s[0:1], s[0:1], 0x24
+; GCN-NEXT:    v_mov_b32_e32 v0, 0
+; GCN-NEXT:    v_mov_b32_e32 v1, 4.0
+; GCN-NEXT:    s_waitcnt vmcnt(0) lgkmcnt(0)
+; GCN-NEXT:    global_atomic_add_f32 v0, v1, s[0:1]
+; GCN-NEXT:    s_waitcnt vmcnt(0)
+; GCN-NEXT:    buffer_wbinvl1_vol
+; GCN-NEXT:    s_endpgm
+  %result = atomicrmw fadd float addrspace(1)* %ptr, float 4.0 syncscope("agent") seq_cst
+  ret void
+}
+
+attributes #0 = { "denormal-fp-math-f32"="preserve-sign,preserve-sign" "target-features"="+atomic-fadd-insts" "amdgpu-unsafe-fp-atomics"="true" }