[PATCH] D65620: [AMDGPU] Use S_DENORM_MODE for gfx10

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:7536
 
+static int getSPDenormMode(bool Enable, bool IsGFX10, const GCNSubtarget *ST) {
+  int SPDenormMode = Enable ? FP_DENORM_FLUSH_NONE
----------------
You do not need IsGFX10 if you have GCNSubtarget.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:7541
+  if (IsGFX10) {
+    int DPDenormDefault = ST->hasFP64Denormals() ? FP_DENORM_FLUSH_NONE
+                                                 : FP_DENORM_FLUSH_IN_FLUSH_OUT;
----------------
Why GFX10 is different here? It is equally controlled by the subtarget attribute on any ASIC.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:7543
+                                                 : FP_DENORM_FLUSH_IN_FLUSH_OUT;
+    SPDenormMode |= DPDenormDefault << 2;
+  }
----------------
I do not think this is right. When you restore mode you need to check current defaults, FP64Denomals and FP32Denormals. Otherwise you are restoring not what it was before.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:7623
+                : DAG.getNode(AMDGPUISD::SETREG, SL, MVT::Other,
+                              Fma4.getValue(1), DisableDenormValue, BitField,
+                              Fma4.getValue(2));
----------------
The values for setreg and s_denorm_mode are different in size, you should not use the same constant, even if you mask it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65620