Page MenuHomePhabricator

[AMDGPU] Use S_DENORM_MODE for gfx10
ClosedPublic

Authored by kerbowa on Aug 1 2019, 4:37 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

kerbowa created this revision.Aug 1 2019, 4:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2019, 4:37 PM
arsenm added inline comments.Aug 1 2019, 4:49 PM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
7536–7547 ↗(On Diff #212928)

The two instructions use different formats, so I would just split this into 2 separate functions for each case rather than checking IsGFX10 inside one

7576 ↗(On Diff #212928)

Should move this check into a Subtarget->hasDenormModeInst()

rampitec added inline comments.Aug 1 2019, 4:57 PM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
7536 ↗(On Diff #212928)

You do not need IsGFX10 if you have GCNSubtarget.

7541 ↗(On Diff #212928)

Why GFX10 is different here? It is equally controlled by the subtarget attribute on any ASIC.

7543 ↗(On Diff #212928)

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.

7623 ↗(On Diff #212928)

The values for setreg and s_denorm_mode are different in size, you should not use the same constant, even if you mask it.

kerbowa marked 2 inline comments as done.Aug 1 2019, 5:27 PM
kerbowa added inline comments.
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
7541 ↗(On Diff #212928)

I will make it into separate functions like Matt is suggesting so it is more clear.

7543 ↗(On Diff #212928)

This is supposed to return the default for FP64FP16 along with either FP_DENORM_FLUSH_NONE or FP_DENORM_FLUSH_IN_FLUSH_OUT for FP32 based on the Enable flag.

rampitec added inline comments.Aug 1 2019, 5:36 PM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
7543 ↗(On Diff #212928)

It is quite misleading, a better code separation is needed.

kerbowa updated this revision to Diff 212967.Aug 1 2019, 9:46 PM

Reorganize.

arsenm added inline comments.Aug 2 2019, 11:43 AM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
7536 ↗(On Diff #212967)

Typo immeidate

7546 ↗(On Diff #212967)

Should use getTargetConstant (or I'll end up fixing this for D58232)

llvm/lib/Target/AMDGPU/SOPInstructions.td
1172 ↗(On Diff #212967)

timm instead of UIMM4bit to match

This revision is now accepted and ready to land.Aug 2 2019, 11:44 AM
kerbowa updated this revision to Diff 213254.Aug 4 2019, 10:06 AM

Switch to timm.

arsenm accepted this revision.Aug 5 2019, 9:03 AM

LGTM. It seems like s_denorm_mode should have special syntax like setreg, but that's a separate patch

This revision was automatically updated to reflect the committed changes.

The test is failing on some bots: http://lab.llvm.org:8011/builders/clang-atom-d525-fedora-rel/builds/27245/steps/ninja%20check%201/logs/FAIL%3A%20LLVM%3A%3Agfx10_dasm_all.txt

******************** TEST 'LLVM :: MC/Disassembler/AMDGPU/gfx10_dasm_all.txt' FAILED ********************
Script:
--
: 'RUN: at line 1';   /export/users/atombot/llvm/clang-atom-d525-fedora-rel/stage1/bin/llvm-mc -arch=amdgcn -mcpu=gfx1010 -mattr=+wavefrontsize32,-wavefrontsize64 -disassemble -show-encoding < /export/users/atombot/llvm/clang-atom-d525-fedora-rel/llvm/test/MC/Disassembler/AMDGPU/gfx10_dasm_all.txt | /export/users/atombot/llvm/clang-atom-d525-fedora-rel/stage1/bin/FileCheck -check-prefixes=GFX10,W32 /export/users/atombot/llvm/clang-atom-d525-fedora-rel/llvm/test/MC/Disassembler/AMDGPU/gfx10_dasm_all.txt
: 'RUN: at line 2';   /export/users/atombot/llvm/clang-atom-d525-fedora-rel/stage1/bin/llvm-mc -arch=amdgcn -mcpu=gfx1010 -mattr=-wavefrontsize32,+wavefrontsize64 -disassemble -show-encoding < /export/users/atombot/llvm/clang-atom-d525-fedora-rel/llvm/test/MC/Disassembler/AMDGPU/gfx10_dasm_all.txt | /export/users/atombot/llvm/clang-atom-d525-fedora-rel/stage1/bin/FileCheck -check-prefixes=GFX10,W64 /export/users/atombot/llvm/clang-atom-d525-fedora-rel/llvm/test/MC/Disassembler/AMDGPU/gfx10_dasm_all.txt
--
Exit Code: 1

Command Output (stderr):
--
/export/users/atombot/llvm/clang-atom-d525-fedora-rel/llvm/test/MC/Disassembler/AMDGPU/gfx10_dasm_all.txt:13619:10: error: GFX10: expected string not found in input
# GFX10: s_denorm_mode 0x0 ; encoding: [0x00,0x00,0xa5,0xbf]
         ^
<stdin>:4540:2: note: scanning from here
 s_denorm_mode 0 ; encoding: [0x00,0x00,0xa5,0xbf]
 ^

Sorry, I reverted this commit, it broke the test MC/Disassembler/AMDGPU/gfx10_dasm_all.txt..

http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/15915

rnk added a subscriber: rnk.Aug 5 2019, 11:46 AM

I tried changing the test expectations to match 0 instead of 0x0 in rL367907, since both my Windows machine and Linux machine agree on that output. If that doesn't work and there is some platform variance, we can always use regexes to match both.

In D65620#1615374, @rnk wrote:

I tried changing the test expectations to match 0 instead of 0x0 in rL367907, since both my Windows machine and Linux machine agree on that output. If that doesn't work and there is some platform variance, we can always use regexes to match both.

Since the commit was reverted it will probably be broken again.