[PATCH] D43414: AMDGPU: Define FP_FAST_FMA{F} macros for amdgcn

[Tony Tye via Phabricator via llvm-commits]

t-tye requested changes to this revision.
t-tye added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Basic/Targets/AMDGPU.cpp:159
 
   // XXX - What does the member GPU mean if device name string passed here?
   if (getTriple().getArch() == llvm::Triple::amdgcn) {
----------------
What does this mean? Has it now been addressed by this patch?


================
Comment at: lib/Basic/Targets/AMDGPU.cpp:276
     SmallVectorImpl<StringRef> &Values) const {
   if (getTriple().getArch() == llvm::Triple::amdgcn)
+    llvm::for_each(AMDGCNGPUs, [&Values](const GPUInfo &GPU) {
----------------
To be consistent should this be:

```
if (isAMDGCN(getTriple()))
```

Similar comment elsewhere.


================
Comment at: lib/Basic/Targets/AMDGPU.cpp:282
-    hasFP64 = true;
-    hasFMAF = true;
-    hasLDEXPF = true;
----------------
This was incorrect in the old code. Only full rate FP64 gcn targets have fast FMAF.


================
Comment at: lib/Basic/Targets/AMDGPU.h:88-91
+    GK_R600_FIRST = GK_R600,
+    GK_R600_LAST = GK_TURKS,
+    GK_AMDGCN_FIRST = GK_GFX600,
+    GK_AMDGCN_LAST = GK_GFX902,
----------------
Would it be better to position these at the beginning/end of the respective enumerations so it is more obvious that they must be updated when adding a new target?


================
Comment at: lib/Basic/Targets/AMDGPU.h:98-102
+    bool HasFMAF;
+    bool HasFP64;
+    bool HasLDEXPF;
+    bool HasFastFMA;
+    bool HasFastFMAF;
----------------
Suggest reordering to be in a logical groups of FP32 and FP64:

```
    bool HasFMAF;
    bool HasFastFMAF;
    bool HasLDEXPF;
    bool HasFP64;
    bool HasFastFMA;
```


================
Comment at: lib/Basic/Targets/AMDGPU.h:108
+  static constexpr GPUInfo R600GPUs[26] = {
+    {{"r600"},    {"r600"},    GK_R600,    false, false, false, false, false},
+    {{"rv630"},   {"r600"},    GK_R600,    false, false, false, false, false},
----------------
Suggest adding a comment here which is a header for the columns to make it easier to check if the settings are right. For example:

```
// Name  Canonical  Kind  HasFMAF   HasFP64   HasLDEXPF   HasFastFMA   HasFastFMAF
```


================
Comment at: lib/Basic/Targets/AMDGPU.h:136
+  static constexpr GPUInfo AMDGCNGPUs[30] = {
+    {{"gfx600"},    {"gfx600"},  GK_GFX600,  true, true, true, true, true},
+    {{"tahiti"},    {"gfx600"},  GK_GFX600,  true, true, true, true, true},
----------------
Same comment as above.

Also,  I think the fast_fma and fast_fmaf columns are reversed. All gcn has fast_fma, it is fast_fmaf that varies.


================
Comment at: lib/Basic/Targets/AMDGPU.h:147
+    {{"hawaii"},    {"gfx701"},  GK_GFX701,  true, true, true, true, true},
+    {{"gfx702"},    {"gfx702"},  GK_GFX702,  true, true, true, true, false},
+    {{"gfx703"},    {"gfx703"},  GK_GFX703,  true, true, true, true, false},
----------------
gfx702 should have true for fast fmaf.

Does the TD file need correcting too?


================
Comment at: lib/Basic/Targets/AMDGPU.h:325
 
-    return GPU.Kind != GK_NONE;
+    return GK_NONE != GPU.Kind;
   }
----------------
I found the original operand order easier to read:-)


https://reviews.llvm.org/D43414