[PATCH] D65620: [AMDGPU] Use S_DENORM_MODE for gfx10
Stanislav Mekhanoshin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 1 16:58:34 PDT 2019
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
More information about the llvm-commits
mailing list