Page MenuHomePhabricator

AMDGPU: Remove denormal subtarget features
Needs ReviewPublic

Authored by arsenm on Dec 11 2019, 6:46 AM.

Details

Reviewers
rampitec
b-sumner
Summary

Switch to using the denormal-fp-math/denormal-fp-math-f32
attributes. Determine the defaults from the calling convention, to
assume flushing in graphics shaders by default.

Diff Detail

Event Timeline

arsenm created this revision.Dec 11 2019, 6:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 11 2019, 6:46 AM

How is it now achieved that we do not support denormals by default before GFX9?

How is it now achieved that we do not support denormals by default before GFX9?

This is a frontend decision. clang emits the attribute based on the subtarget preference

How is it now achieved that we do not support denormals by default before GFX9?

This is a frontend decision. clang emits the attribute based on the subtarget preference

This is also how it always worked. The backend uniformly treated denormals as off by default

The code LGTM. But I want Brian to review it.

How is it now achieved that we do not support denormals by default before GFX9?

This is a frontend decision. clang emits the attribute based on the subtarget preference

This is also how it always worked. The backend uniformly treated denormals as off by default

Where is clang going to get the information it needs after this patch?

Would it be possible to do this patch in two steps:

  1. First step: Add the parsing of the new function attribute, but keep the subtarget feature.
  2. Second step: Remove the subtarget feature.

This would make the upgrade process easier for out-of-tree users (Mesa, LLPC).

Apart from that this seems like a good change to me.

How is it now achieved that we do not support denormals by default before GFX9?

This is a frontend decision. clang emits the attribute based on the subtarget preference

This is also how it always worked. The backend uniformly treated denormals as off by default

Where is clang going to get the information it needs after this patch?

Where it already does. This doesn’t change. It just changes what clang produces for the denormal flushing flag/subtarget default

Would it be possible to do this patch in two steps:

  1. First step: Add the parsing of the new function attribute, but keep the subtarget feature.
  2. Second step: Remove the subtarget feature.

    This would make the upgrade process easier for out-of-tree users (Mesa, LLPC).

    Apart from that this seems like a good change to me.

That is basically how it’s split. Once the form of the attributes is finalized, frontends can start emitting the new form and we just won’t look it it until this patch

Once the form of the attributes is finalized, frontends can start emitting the new form and we just won’t look it it until this patch

Okay, that makes sense.

I am OK with proceeding here.

mareko added a subscriber: mareko.Jan 6 2020, 12:31 PM

Can you push this after (not before) LLVM 10 has been branched?