Page MenuHomePhabricator

Separately track input and output denormal mode
ClosedPublic

Authored by arsenm on Nov 7 2019, 5:11 PM.

Details

Summary

AMDGPU and x86 at least both have separate controls for whether
denormal results are flushed on output, and for whether denormals are
implicitly treated as 0 as an input. The current DAGCombiner use only
really cares about the input treatment of denormals.

Diff Detail

Event Timeline

arsenm created this revision.Nov 7 2019, 5:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 7 2019, 5:11 PM
llvm/docs/LangRef.rst
1821–1822

I don't like the definition of this attribute. It's not reader-friendly. The comma-separated pair format has no indication which value refers to inputs and which refers to outputs. Also, while this predates your changes, I think the meanings of the current choices are unclear.

What would you think of a comma-separated list with the following possibilities?

allow-denormals (default)
inputs-are-zero (outputs not flushed)
inputs-are-zero, outputs-are-zero
inputs-are-zero, outputs-are-positive-zero
inputs-are-positivezero (outputs not flushed)
inputs-are-positivezero, outputs-are-zero
inputs-are-positivezero, outputs-are-positive-zero
denormal-outputs-are-zero (inputs are unchanged)
denormal-outputs-are-positive-zero (inputs are unchanged)

I'd also be open to abbreviations. I don't know if "daz" and "ftz" are readable to everyone, but I'm more comfortable with them. That would make the options something like this.

allow-denormals
daz
daz, ftz
daz, ftz+
daz+
daz+, ftz
daz+, ftz+
ftz
ftz+
arsenm marked an inline comment as done.Nov 18 2019, 3:47 AM
arsenm added inline comments.
llvm/docs/LangRef.rst
1821–1822

I'm trying to avoid needing to autoupgrade bitcode at this point, which leaving the names as-is accomplishes. I'm worried this could still end up not in the right place, and then we would need another level of auto upgrade to deal with it later. I think these are overly verbose (I'm also keeping in mind the fact that any use of these does a linear scan through all string attributes, and then needs to parse these).

I'm also unclear on what this weird ARM positive-zero really means. Does it mean inputs and outputs ignored the sign? Is there value in representing positive-zero on both sides?

pengfei added a subscriber: pengfei.Dec 2 2019, 4:43 PM
llvm/docs/LangRef.rst
1829

Based on the changes below, if the second value is omitted the input mode will be assumed to be the same as the output mode. That should probably be documented. I guess you intend for that not to happen, but the documentation here leaves the result ambiguous if it does happen.

1850

Is this saying that if a backend generates an instruction that doesn't handle the hardware daz mode then it must insert instructions to check for normals and convert them to zero? If so, do you intend this to apply to all such instructions or only instructions that aren't able to accept denormal inputs?

nhaehnle removed a subscriber: nhaehnle.Jan 30 2020, 1:25 AM
arsenm marked 3 inline comments as done.Jan 31 2020, 6:18 AM
arsenm added inline comments.
llvm/docs/LangRef.rst
1829

I've added a note for this

1850

Only in cases where denormal inputs are invalid or unhandled. The case I'm thinking of is the one user in DAGCombiner, where if denormals are not flushed the result ends up incorrect (see https://bugs.llvm.org/show_bug.cgi?id=34994)

arsenm updated this revision to Diff 241714.Jan 31 2020, 6:20 AM
arsenm marked an inline comment as done.

Tweak langref

This revision is now accepted and ready to land.Jan 31 2020, 11:46 AM