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

[Konstantin Zhuravlyov via Phabricator via llvm-commits]

kzhuravl added inline comments.


================
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) {
----------------
t-tye wrote:
> What does this mean? Has it now been addressed by this patch?
This class has a member called *GPU*. We are using the *CPU* that is passed as an argument.

> Has it now been addressed by this patch?
No.


================
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},
----------------
t-tye wrote:
> 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.
fast_fmaf is the last column. I think those are in the correct order.


================
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},
----------------
t-tye wrote:
> gfx702 should have true for fast fmaf.
> 
> Does the TD file need correcting too?
Not according to the TD files in our BE: https://github.com/llvm-mirror/llvm/blob/master/lib/Target/AMDGPU/AMDGPU.td#L545


================
Comment at: lib/Basic/Targets/AMDGPU.h:325
 
-    return GPU.Kind != GK_NONE;
+    return GK_NONE != GPU.Kind;
   }
----------------
t-tye wrote:
> I found the original operand order easier to read:-)
I was trying to match the style above. But I can change it back.


https://reviews.llvm.org/D43414