This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][GFX9] Set +fp32-denormals for >=gfx900 unless -cl-denorms-are-zero is set
ClosedPublic

Authored by kzhuravl on Mar 29 2017, 3:51 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

kzhuravl created this revision.Mar 29 2017, 3:51 PM
arsenm added inline comments.Mar 29 2017, 3:54 PM
llvm/tools/clang/lib/Basic/Targets.cpp
2114–2116 ↗(On Diff #93420)

This is misleading since it was true on VI as well. I think just FMA rate changed

kzhuravl added inline comments.Mar 29 2017, 4:06 PM
llvm/tools/clang/lib/Basic/Targets.cpp
2114–2116 ↗(On Diff #93420)

Yes, GFX8 supports f32 denorms at full speed too. However, it doesn't have a full speed fma, so we didh't enable it then since it caused too many mad-heavy apps to slow down.

arsenm added inline comments.Mar 29 2017, 4:11 PM
llvm/tools/clang/lib/Basic/Targets.cpp
2114–2116 ↗(On Diff #93420)

Yes, so the name should refer to FMA rather than just fp32 denorms

kzhuravl updated this revision to Diff 93422.Mar 29 2017, 4:31 PM
kzhuravl marked an inline comment as done.

Address review feedback.

arsenm accepted this revision.Mar 29 2017, 5:55 PM

LGTM with f32 clarification

lib/Basic/Targets.cpp
2114 ↗(On Diff #93422)

FMAF32?

2115 ↗(On Diff #93422)

We should probably add a new subtarget feature for this, but that's a separate patch

This revision is now accepted and ready to land.Mar 29 2017, 5:55 PM
arsenm added inline comments.Mar 29 2017, 5:57 PM
lib/Basic/Targets.cpp
2114 ↗(On Diff #93422)

Actually this also needs to specify full speed FMA with denorms. Full rate FMA is already a subtarget feature when denorms are disabled

This revision was automatically updated to reflect the committed changes.
kzhuravl marked 2 inline comments as done.
kzhuravl added inline comments.Apr 13 2017, 10:49 PM
lib/Basic/Targets.cpp
2114 ↗(On Diff #93422)

I will do it in a separate patch. Thanks.